From a807ee0a4d4f962e3cff45a1b6128ebc9897aee1 Mon Sep 17 00:00:00 2001 From: Fabian Gonzalez Date: Sat, 23 Nov 2024 11:35:49 -0500 Subject: [PATCH 1/5] do not ignore errors on decode --- datamodel/spec_info.go | 44 ++++++++++++++++++++++++++-------- datamodel/spec_info_test.go | 48 +++++++++++++++++++++++++++---------- 2 files changed, 69 insertions(+), 23 deletions(-) diff --git a/datamodel/spec_info.go b/datamodel/spec_info.go index 28f429af..1bb1880f 100644 --- a/datamodel/spec_info.go +++ b/datamodel/spec_info.go @@ -88,18 +88,30 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro _, openAPI2 := utils.FindKeyNode(utils.OpenApi2, parsedSpec.Content) _, asyncAPI := utils.FindKeyNode(utils.AsyncApi, parsedSpec.Content) - parseJSON := func(bytes []byte, spec *SpecInfo, parsedNode *yaml.Node) { + parseJSON := func(bytes []byte, spec *SpecInfo, parsedNode *yaml.Node) error { var jsonSpec map[string]interface{} + var errs []error if utils.IsYAML(string(bytes)) { - _ = parsedNode.Decode(&jsonSpec) - b, _ := json.Marshal(&jsonSpec) + err := parsedNode.Decode(&jsonSpec) + if err != nil { + errs = append(errs, err) + } + b, err := json.Marshal(&jsonSpec) + if err != nil { + errs = append(errs, err) + } spec.SpecJSONBytes = &b spec.SpecJSON = &jsonSpec } else { - _ = json.Unmarshal(bytes, &jsonSpec) + err := json.Unmarshal(bytes, &jsonSpec) + if err != nil { + errs = append(errs, err) + } spec.SpecJSONBytes = &bytes spec.SpecJSON = &jsonSpec } + + return errors.Join(errs...) } if !bypass { @@ -125,7 +137,10 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro } // parse JSON - parseJSON(spec, specInfo, &parsedSpec) + err := parseJSON(spec, specInfo, &parsedSpec) + if err != nil { + return nil, err + } // double check for the right version, people mix this up. if majorVersion < 3 { @@ -147,7 +162,10 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro specInfo.APISchema = OpenAPI2SchemaData // parse JSON - parseJSON(spec, specInfo, &parsedSpec) + err := parseJSON(spec, specInfo, &parsedSpec) + if err != nil { + return nil, err + } // I am not certain this edge-case is very frequent, but let's make sure we handle it anyway. if majorVersion > 2 { @@ -166,7 +184,10 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro // TODO: format for AsyncAPI. // parse JSON - parseJSON(spec, specInfo, &parsedSpec) + err := parseJSON(spec, specInfo, &parsedSpec) + if err != nil { + return nil, err + } // so far there is only 2 as a major release of AsyncAPI if majorVersion > 2 { @@ -177,13 +198,16 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro if specInfo.SpecType == "" { // parse JSON - parseJSON(spec, specInfo, &parsedSpec) + err := parseJSON(spec, specInfo, &parsedSpec) + if err != nil { + return nil, err + } specInfo.Error = errors.New("spec type not supported by libopenapi, sorry") return specInfo, specInfo.Error } } else { - // parse JSON - parseJSON(spec, specInfo, &parsedSpec) + // parse JSON, ignoring any errors loading the document + _ = parseJSON(spec, specInfo, &parsedSpec) } // detect the original whitespace indentation diff --git a/datamodel/spec_info_test.go b/datamodel/spec_info_test.go index 5ef6ec03..9bc3b9ef 100644 --- a/datamodel/spec_info_test.go +++ b/datamodel/spec_info_test.go @@ -5,10 +5,10 @@ package datamodel import ( "fmt" + "github.com/pb33f/libopenapi/utils" "os" "testing" - "github.com/pb33f/libopenapi/utils" "github.com/stretchr/testify/assert" ) @@ -74,6 +74,23 @@ tags: servers: - url: https://quobix.com/api` +// This is a bad yaml file, where the second openapi path is indented incorrectly, +// causing its 'get' operation to be on the same indent level as the first path's 'get' operation, so it's a duplicate key and would fail decoding. +var BadYAML2 = `openapi: 3.0.1 +info: + title: Test API +paths: + /test: + get: + responses: + '200': + description: OK + /test2: + get: + responses: + '200': + description: OK` + var OpenApi2Spec = `swagger: 2.0.1 info: title: Test API @@ -134,6 +151,11 @@ func TestExtractSpecInfo_InvalidYAML(t *testing.T) { assert.Error(t, e) } +func TestExtractSpecInfo_InvalidYAML2(t *testing.T) { + _, e := ExtractSpecInfo([]byte(BadYAML2)) + assert.Error(t, e) +} + func TestExtractSpecInfo_InvalidOpenAPIVersion(t *testing.T) { _, e := ExtractSpecInfo([]byte(OpenApiOne)) assert.Error(t, e) @@ -166,10 +188,10 @@ func TestExtractSpecInfo_OpenAPI31(t *testing.T) { func TestExtractSpecInfo_AnyDocument(t *testing.T) { random := `something: yeah nothing: - - one - - two + - one + - two why: - yes: no` + yes: no` r, e := ExtractSpecInfoWithDocumentCheck([]byte(random), true) assert.Nil(t, e) @@ -181,10 +203,10 @@ why: func TestExtractSpecInfo_AnyDocument_Sync(t *testing.T) { random := `something: yeah nothing: - - one - - two + - one + - two why: - yes: no` + yes: no` r, e := ExtractSpecInfoWithDocumentCheckSync([]byte(random), true) assert.Nil(t, e) @@ -206,10 +228,10 @@ func TestExtractSpecInfo_AnyDocument_JSON(t *testing.T) { func TestExtractSpecInfo_AnyDocumentFromConfig(t *testing.T) { random := `something: yeah nothing: - - one - - two + - one + - two why: - yes: no` + yes: no` r, e := ExtractSpecInfoWithConfig([]byte(random), &DocumentConfiguration{ BypassDocumentCheck: true, @@ -259,7 +281,7 @@ func TestExtractSpecInfo_AsyncAPI_OddVersion(t *testing.T) { func TestExtractSpecInfo_BadVersion_OpenAPI3(t *testing.T) { yml := `openapi: - should: fail` +should: fail` _, err := ExtractSpecInfo([]byte(yml)) assert.Error(t, err) @@ -267,7 +289,7 @@ func TestExtractSpecInfo_BadVersion_OpenAPI3(t *testing.T) { func TestExtractSpecInfo_BadVersion_Swagger(t *testing.T) { yml := `swagger: - should: fail` +should: fail` _, err := ExtractSpecInfo([]byte(yml)) assert.Error(t, err) @@ -275,7 +297,7 @@ func TestExtractSpecInfo_BadVersion_Swagger(t *testing.T) { func TestExtractSpecInfo_BadVersion_AsyncAPI(t *testing.T) { yml := `asyncapi: - should: fail` +should: fail` _, err := ExtractSpecInfo([]byte(yml)) assert.Error(t, err) From e3fb58551f45a4e036e112b1b038f70140b87823 Mon Sep 17 00:00:00 2001 From: Fabian Gonzalez Date: Sat, 23 Nov 2024 11:37:52 -0500 Subject: [PATCH 2/5] undo format changes --- datamodel/spec_info_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/datamodel/spec_info_test.go b/datamodel/spec_info_test.go index 9bc3b9ef..3abc65fa 100644 --- a/datamodel/spec_info_test.go +++ b/datamodel/spec_info_test.go @@ -188,10 +188,10 @@ func TestExtractSpecInfo_OpenAPI31(t *testing.T) { func TestExtractSpecInfo_AnyDocument(t *testing.T) { random := `something: yeah nothing: - - one - - two + - one + - two why: - yes: no` + yes: no` r, e := ExtractSpecInfoWithDocumentCheck([]byte(random), true) assert.Nil(t, e) @@ -203,10 +203,10 @@ why: func TestExtractSpecInfo_AnyDocument_Sync(t *testing.T) { random := `something: yeah nothing: - - one - - two + - one + - two why: - yes: no` + yes: no` r, e := ExtractSpecInfoWithDocumentCheckSync([]byte(random), true) assert.Nil(t, e) @@ -228,10 +228,10 @@ func TestExtractSpecInfo_AnyDocument_JSON(t *testing.T) { func TestExtractSpecInfo_AnyDocumentFromConfig(t *testing.T) { random := `something: yeah nothing: - - one - - two + - one + - two why: - yes: no` + yes: no` r, e := ExtractSpecInfoWithConfig([]byte(random), &DocumentConfiguration{ BypassDocumentCheck: true, @@ -281,7 +281,7 @@ func TestExtractSpecInfo_AsyncAPI_OddVersion(t *testing.T) { func TestExtractSpecInfo_BadVersion_OpenAPI3(t *testing.T) { yml := `openapi: -should: fail` + should: fail` _, err := ExtractSpecInfo([]byte(yml)) assert.Error(t, err) @@ -289,7 +289,7 @@ should: fail` func TestExtractSpecInfo_BadVersion_Swagger(t *testing.T) { yml := `swagger: -should: fail` + should: fail` _, err := ExtractSpecInfo([]byte(yml)) assert.Error(t, err) @@ -297,7 +297,7 @@ should: fail` func TestExtractSpecInfo_BadVersion_AsyncAPI(t *testing.T) { yml := `asyncapi: -should: fail` + should: fail` _, err := ExtractSpecInfo([]byte(yml)) assert.Error(t, err) From 29e41aa6af260b87470df98d65ac60e1bcb06cbd Mon Sep 17 00:00:00 2001 From: Fabian Gonzalez Date: Sat, 23 Nov 2024 11:39:19 -0500 Subject: [PATCH 3/5] in-line err check --- datamodel/spec_info.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/datamodel/spec_info.go b/datamodel/spec_info.go index 1bb1880f..0622d83b 100644 --- a/datamodel/spec_info.go +++ b/datamodel/spec_info.go @@ -137,8 +137,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro } // parse JSON - err := parseJSON(spec, specInfo, &parsedSpec) - if err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { return nil, err } @@ -162,8 +161,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro specInfo.APISchema = OpenAPI2SchemaData // parse JSON - err := parseJSON(spec, specInfo, &parsedSpec) - if err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { return nil, err } @@ -184,8 +182,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro // TODO: format for AsyncAPI. // parse JSON - err := parseJSON(spec, specInfo, &parsedSpec) - if err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { return nil, err } @@ -198,8 +195,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro if specInfo.SpecType == "" { // parse JSON - err := parseJSON(spec, specInfo, &parsedSpec) - if err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { return nil, err } specInfo.Error = errors.New("spec type not supported by libopenapi, sorry") From bf727e84bf1c956f09e6526eb4a122d384d3c625 Mon Sep 17 00:00:00 2001 From: Fabian Gonzalez Date: Mon, 25 Nov 2024 14:52:34 -0500 Subject: [PATCH 4/5] err handling + added invalid json test --- datamodel/spec_info.go | 23 +++++++++-------------- datamodel/spec_info_test.go | 18 ++++++++++++------ 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/datamodel/spec_info.go b/datamodel/spec_info.go index 0622d83b..708a6e3b 100644 --- a/datamodel/spec_info.go +++ b/datamodel/spec_info.go @@ -90,28 +90,23 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro parseJSON := func(bytes []byte, spec *SpecInfo, parsedNode *yaml.Node) error { var jsonSpec map[string]interface{} - var errs []error + var parseErr error + if utils.IsYAML(string(bytes)) { - err := parsedNode.Decode(&jsonSpec) - if err != nil { - errs = append(errs, err) - } - b, err := json.Marshal(&jsonSpec) - if err != nil { - errs = append(errs, err) - } + parseErr = parsedNode.Decode(&jsonSpec) + // NOTE: Even if Decoding results in an error, `jsonSpec` can still contain partial or empty data. + // This subsequent Marshalling should succeed unless `jsonSpec` contains unsupported types, + // which isn't possible as Decoding will only decode valid data. + b, _ := json.Marshal(&jsonSpec) spec.SpecJSONBytes = &b spec.SpecJSON = &jsonSpec } else { - err := json.Unmarshal(bytes, &jsonSpec) - if err != nil { - errs = append(errs, err) - } + parseErr = json.Unmarshal(bytes, &jsonSpec) spec.SpecJSONBytes = &bytes spec.SpecJSON = &jsonSpec } - return errors.Join(errs...) + return parseErr } if !bypass { diff --git a/datamodel/spec_info_test.go b/datamodel/spec_info_test.go index 3abc65fa..e6a74a63 100644 --- a/datamodel/spec_info_test.go +++ b/datamodel/spec_info_test.go @@ -24,9 +24,10 @@ const ( ) var ( - goodJSON = `{"name":"kitty", "noises":["meow","purrrr","gggrrraaaaaooooww"]}` - badJSON = `{"name":"kitty, "noises":[{"meow","purrrr","gggrrraaaaaooooww"]}}` - goodYAML = `name: kitty + goodJSON = `{"name":"kitty", "noises":["meow","purrrr","gggrrraaaaaooooww"]}` + badJSON = `{"name":"kitty, "noises":[{"meow","purrrr","gggrrraaaaaooooww"]}}` + badJSONMissingQuote = `{"openapi": "3.0.1", "name":"kitty", "name":["kitty2",kitty3"]}` + goodYAML = `name: kitty noises: - meow - purrr @@ -76,7 +77,7 @@ servers: // This is a bad yaml file, where the second openapi path is indented incorrectly, // causing its 'get' operation to be on the same indent level as the first path's 'get' operation, so it's a duplicate key and would fail decoding. -var BadYAML2 = `openapi: 3.0.1 +var BadYAMLRepeatedPathKey = `openapi: 3.0.1 info: title: Test API paths: @@ -135,6 +136,11 @@ func TestExtractSpecInfo_InvalidJSON(t *testing.T) { assert.Error(t, e) } +func TestExtractSpecInfo_InvalidJSON_MissingQuote(t *testing.T) { + _, e := ExtractSpecInfo([]byte(badJSONMissingQuote)) + assert.Error(t, e) +} + func TestExtractSpecInfo_Nothing(t *testing.T) { _, e := ExtractSpecInfo([]byte("")) assert.Error(t, e) @@ -151,8 +157,8 @@ func TestExtractSpecInfo_InvalidYAML(t *testing.T) { assert.Error(t, e) } -func TestExtractSpecInfo_InvalidYAML2(t *testing.T) { - _, e := ExtractSpecInfo([]byte(BadYAML2)) +func TestExtractSpecInfo_InvalidYAML_RepeatedPathKey(t *testing.T) { + _, e := ExtractSpecInfo([]byte(BadYAMLRepeatedPathKey)) assert.Error(t, e) } From 2e1dde3252054bcb08622b5e0a40ea25a4631069 Mon Sep 17 00:00:00 2001 From: Fabian Gonzalez Date: Mon, 25 Nov 2024 15:12:16 -0500 Subject: [PATCH 5/5] only return err if not bypassing --- datamodel/spec_info.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/datamodel/spec_info.go b/datamodel/spec_info.go index d9b1791d..3e1c6fa5 100644 --- a/datamodel/spec_info.go +++ b/datamodel/spec_info.go @@ -140,7 +140,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro } // parse JSON - if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil && !bypass { return nil, err } parsed = true @@ -169,7 +169,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro specInfo.APISchema = OpenAPI2SchemaData // parse JSON - if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil && !bypass { return nil, err } parsed = true @@ -195,7 +195,7 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro // TODO: format for AsyncAPI. // parse JSON - if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil && !bypass { return nil, err } parsed = true @@ -226,7 +226,9 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro //} if !parsed { - _ = parseJSON(spec, specInfo, &parsedSpec) + if err := parseJSON(spec, specInfo, &parsedSpec); err != nil && !bypass { + return nil, err + } } // detect the original whitespace indentation