From dd9f2425fd7db115917abcc39680d8bcba0870e6 Mon Sep 17 00:00:00 2001 From: sorintm Date: Fri, 23 Sep 2022 17:24:27 +0100 Subject: [PATCH 1/5] Add repro for issue 615 --- openapi3/loader_recursive_ref_test.go | 9 ++++ openapi3/testdata/recursiveRef/issue615.yml | 60 +++++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 openapi3/testdata/recursiveRef/issue615.yml diff --git a/openapi3/loader_recursive_ref_test.go b/openapi3/loader_recursive_ref_test.go index 5b7c1506e..9f6202023 100644 --- a/openapi3/loader_recursive_ref_test.go +++ b/openapi3/loader_recursive_ref_test.go @@ -16,6 +16,15 @@ func TestLoaderSupportsRecursiveReference(t *testing.T) { require.Equal(t, "bar", doc.Paths["/foo"].Get.Responses.Get(200).Value.Content.Get("application/json").Schema.Value.Properties["foo2"].Value.Properties["foo"].Value.Properties["bar"].Value.Example) } +func TestIssue615(t *testing.T) { + loader := NewLoader() + loader.IsExternalRefsAllowed = true + _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") + // Test currently reproduces the issue 615: failure to load a valid spec + // Upon issue resolution, this check should be changed to require.NoError + require.Error(t, err) +} + func TestIssue447(t *testing.T) { loader := NewLoader() doc, err := loader.LoadFromData([]byte(` diff --git a/openapi3/testdata/recursiveRef/issue615.yml b/openapi3/testdata/recursiveRef/issue615.yml new file mode 100644 index 000000000..d1370e32e --- /dev/null +++ b/openapi3/testdata/recursiveRef/issue615.yml @@ -0,0 +1,60 @@ +openapi: "3.0.3" +info: + title: Deep recursive cyclic refs example + version: "1.0" +paths: + /foo: + $ref: ./paths/foo.yml +components: + schemas: + FilterColumnIncludes: + type: object + properties: + $includes: + $ref: '#/components/schemas/FilterPredicate' + additionalProperties: false + maxProperties: 1 + minProperties: 1 + FilterPredicate: + oneOf: + - $ref: '#/components/schemas/FilterValue' + - type: array + items: + $ref: '#/components/schemas/FilterPredicate' + minLength: 1 + - $ref: '#/components/schemas/FilterPredicateOp' + - $ref: '#/components/schemas/FilterPredicateRangeOp' + FilterPredicateOp: + type: object + properties: + $any: + oneOf: + - type: array + items: + $ref: '#/components/schemas/FilterPredicate' + $none: + oneOf: + - $ref: '#/components/schemas/FilterPredicate' + - type: array + items: + $ref: '#/components/schemas/FilterPredicate' + additionalProperties: false + maxProperties: 1 + minProperties: 1 + FilterPredicateRangeOp: + type: object + properties: + $lt: + $ref: '#/components/schemas/FilterRangeValue' + additionalProperties: false + maxProperties: 2 + minProperties: 2 + FilterRangeValue: + oneOf: + - type: number + - type: string + FilterValue: + oneOf: + - type: number + - type: string + - type: boolean \ No newline at end of file From 1d513bca0db37f10a61db2487f81ac419c420fd3 Mon Sep 17 00:00:00 2001 From: Pierre Fenoll Date: Fri, 7 Oct 2022 18:15:21 +0200 Subject: [PATCH 2/5] move test to dedicated package and introduce tunable limit Signed-off-by: Pierre Fenoll --- openapi3/issue615_test.go | 27 +++++++++++++++++++++++++++ openapi3/loader.go | 7 ++++--- openapi3/loader_recursive_ref_test.go | 9 --------- 3 files changed, 31 insertions(+), 12 deletions(-) create mode 100644 openapi3/issue615_test.go diff --git a/openapi3/issue615_test.go b/openapi3/issue615_test.go new file mode 100644 index 000000000..12ad015ae --- /dev/null +++ b/openapi3/issue615_test.go @@ -0,0 +1,27 @@ +package openapi3_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/getkin/kin-openapi/openapi3" +) + +func TestIssue615(t *testing.T) { + loader := openapi3.NewLoader() + loader.IsExternalRefsAllowed = true + _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") + // Test currently reproduces the issue 615: failure to load a valid spec + // Upon issue resolution, this check should be changed to require.NoError + require.Error(t, err, openapi3.CircularReferenceError) + + var old int + old, openapi3.CircularReferenceCounter = openapi3.CircularReferenceCounter, 4 + defer func() { openapi3.CircularReferenceCounter = old }() + + loader = openapi3.NewLoader() + loader.IsExternalRefsAllowed = true + _, err = loader.LoadFromFile("testdata/recursiveRef/issue615.yml") + require.NoError(t, err) +} diff --git a/openapi3/loader.go b/openapi3/loader.go index 637e1acf9..0c083312d 100644 --- a/openapi3/loader.go +++ b/openapi3/loader.go @@ -17,6 +17,7 @@ import ( ) var CircularReferenceError = "kin-openapi bug found: circular schema reference not handled" +var CircularReferenceCounter = 3 func foundUnresolvedRef(ref string) error { return fmt.Errorf("found unresolved ref: %q", ref) @@ -724,7 +725,7 @@ func (loader *Loader) resolveSchemaRef(doc *T, component *SchemaRef, documentPat } component.Value = &schema } else { - if visitedLimit(visited, ref, 3) { + if visitedLimit(visited, ref) { visited = append(visited, ref) return fmt.Errorf("%s - %s", CircularReferenceError, strings.Join(visited, " -> ")) } @@ -1088,12 +1089,12 @@ func unescapeRefString(ref string) string { return strings.Replace(strings.Replace(ref, "~1", "/", -1), "~0", "~", -1) } -func visitedLimit(visited []string, ref string, limit int) bool { +func visitedLimit(visited []string, ref string) bool { visitedCount := 0 for _, v := range visited { if v == ref { visitedCount++ - if visitedCount >= limit { + if visitedCount >= CircularReferenceCounter { return true } } diff --git a/openapi3/loader_recursive_ref_test.go b/openapi3/loader_recursive_ref_test.go index 99ab3ad1e..924cb6be8 100644 --- a/openapi3/loader_recursive_ref_test.go +++ b/openapi3/loader_recursive_ref_test.go @@ -18,15 +18,6 @@ func TestLoaderSupportsRecursiveReference(t *testing.T) { require.Equal(t, "ErrorDetails", doc.Paths["/double-ref-foo"].Get.Responses.Get(400).Value.Content.Get("application/json").Schema.Value.Title) } -func TestIssue615(t *testing.T) { - loader := NewLoader() - loader.IsExternalRefsAllowed = true - _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") - // Test currently reproduces the issue 615: failure to load a valid spec - // Upon issue resolution, this check should be changed to require.NoError - require.Error(t, err) -} - func TestIssue447(t *testing.T) { loader := NewLoader() doc, err := loader.LoadFromData([]byte(` From ae20d7ae81702ce0c07a56787a9b7e57ce236bd8 Mon Sep 17 00:00:00 2001 From: Pierre Fenoll Date: Fri, 7 Oct 2022 18:27:20 +0200 Subject: [PATCH 3/5] ensure test fails successfully Signed-off-by: Pierre Fenoll --- openapi3/issue615_test.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/openapi3/issue615_test.go b/openapi3/issue615_test.go index 12ad015ae..919bed9e1 100644 --- a/openapi3/issue615_test.go +++ b/openapi3/issue615_test.go @@ -9,19 +9,24 @@ import ( ) func TestIssue615(t *testing.T) { - loader := openapi3.NewLoader() - loader.IsExternalRefsAllowed = true - _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") - // Test currently reproduces the issue 615: failure to load a valid spec - // Upon issue resolution, this check should be changed to require.NoError - require.Error(t, err, openapi3.CircularReferenceError) + for { + loader := openapi3.NewLoader() + loader.IsExternalRefsAllowed = true + _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") + // Test currently reproduces the issue 615: failure to load a valid spec + // Upon issue resolution, this check should be changed to require.NoError + require.Error(t, err, openapi3.CircularReferenceError) + if err != nil { + break + } + } var old int old, openapi3.CircularReferenceCounter = openapi3.CircularReferenceCounter, 4 defer func() { openapi3.CircularReferenceCounter = old }() - loader = openapi3.NewLoader() + loader := openapi3.NewLoader() loader.IsExternalRefsAllowed = true - _, err = loader.LoadFromFile("testdata/recursiveRef/issue615.yml") + _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") require.NoError(t, err) } From 0c9b4d90d95e23b44afa1b5055e37f9f5e2524c4 Mon Sep 17 00:00:00 2001 From: Pierre Fenoll Date: Fri, 7 Oct 2022 18:27:25 +0200 Subject: [PATCH 4/5] test harder Signed-off-by: Pierre Fenoll --- .github/workflows/go.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index d34061a04..649a14846 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -62,10 +62,10 @@ jobs: - run: git --no-pager diff --exit-code - if: runner.os == 'Linux' - run: go test ./... + run: go test -count=10 ./... env: GOARCH: '386' - - run: go test ./... + - run: go test -count=10 ./... - run: go test -count=2 -covermode=atomic ./... - run: go test -v -run TestRaceyPatternSchema -race ./... env: From 4b707003206cca5ed111f84832b1039f0fe59d3d Mon Sep 17 00:00:00 2001 From: Pierre Fenoll Date: Fri, 7 Oct 2022 18:40:50 +0200 Subject: [PATCH 5/5] come on now Signed-off-by: Pierre Fenoll --- openapi3/issue615_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openapi3/issue615_test.go b/openapi3/issue615_test.go index 919bed9e1..ceb317ab0 100644 --- a/openapi3/issue615_test.go +++ b/openapi3/issue615_test.go @@ -13,12 +13,13 @@ func TestIssue615(t *testing.T) { loader := openapi3.NewLoader() loader.IsExternalRefsAllowed = true _, err := loader.LoadFromFile("testdata/recursiveRef/issue615.yml") + if err == nil { + continue + } // Test currently reproduces the issue 615: failure to load a valid spec // Upon issue resolution, this check should be changed to require.NoError require.Error(t, err, openapi3.CircularReferenceError) - if err != nil { - break - } + break } var old int