From b17a8a30c77e3942f90dcf14725ae034d05e3789 Mon Sep 17 00:00:00 2001 From: Adam Mohammed Date: Wed, 25 Sep 2019 18:42:29 -0400 Subject: [PATCH 1/3] Check Content-Type before unmarshalling json This addresses the issue #84. WHen the API servers were returned a 502, linodego paniced. This happened due to the client trying to parse the response as json when a `text/html` response was received. The solution is to check the response content-type to see if it matches the content-type accepted by the request. All of the code base assumes that the response is well-formed json, so it may be okay to just check to see if the content-type matches "application/json". If you remove the code added in `errors.go` the included test will fail due to panicing when trying to unmarshal the html response. --- client_test.go | 14 +++++ errors.go | 8 +++ .../TestClient_APIResponseBadGateway.yaml | 62 +++++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 fixtures/TestClient_APIResponseBadGateway.yaml diff --git a/client_test.go b/client_test.go index c16b1a3bf..8280c4389 100644 --- a/client_test.go +++ b/client_test.go @@ -1,6 +1,7 @@ package linodego_test import ( + "context" "log" "net/http" "os" @@ -127,3 +128,16 @@ func TestClientAliases(t *testing.T) { t.Error("Expected alias for Volumes to return a *Resource") } } + +func TestClient_APIResponseBadGateway(t *testing.T) { + client, teardown := createTestClient(t, "fixtures/TestClient_APIResponseBadGateway.yaml") + defer teardown() + + defer func() { + if r := recover(); r != nil { + t.Errorf("Expected Client to handle 502 from API Server") + } + }() + client.ListImages(context.Background(), nil) + +} diff --git a/errors.go b/errors.go index feacc2cb0..e0789d6cb 100644 --- a/errors.go +++ b/errors.go @@ -49,6 +49,14 @@ func coupleAPIErrors(r *resty.Response, err error) (*resty.Response, error) { } if r.Error() != nil { + // Check that response is of the correct content-type before unmarshalling + expectedContentType := r.Request.Header.Get("Accept") + responseContentType := r.Header().Get("Content-Type") + if responseContentType != expectedContentType { + msg := fmt.Sprintf("Expected: %v; Received: %v", expectedContentType, responseContentType) + return nil, NewError(msg) + } + apiError, ok := r.Error().(*APIError) if !ok || (ok && len(apiError.Errors) == 0) { return r, nil diff --git a/fixtures/TestClient_APIResponseBadGateway.yaml b/fixtures/TestClient_APIResponseBadGateway.yaml new file mode 100644 index 000000000..82c4cf0ce --- /dev/null +++ b/fixtures/TestClient_APIResponseBadGateway.yaml @@ -0,0 +1,62 @@ +--- +version: 1 +interactions: +- request: + body: "" + form: {} + headers: + Accept: + - application/json + Authorization: + - Bearer awesometokenawesometokenawesometoken + Content-Type: + - application/json + User-Agent: + - linodego 0.0.1 https://github.com/linode/linodego + url: https://api.linode.com/v4/linode/instances/1231 + method: GET + response: + body: ' + 502 Bad Gateway +

Bad Gateway

+

The proxy server received an invalid response from an upstream server.

' + headers: + Access-Control-Allow-Headers: + - Authorization, Origin, X-Requested-With, Content-Type, Accept, X-Filter + Access-Control-Allow-Methods: + - HEAD, GET, OPTIONS, POST, PUT, DELETE + Access-Control-Allow-Origin: + - '*' + Cache-Control: + - private, max-age=0, s-maxage=0, no-cache, no-store + Connection: + - keep-alive + Content-Length: + - "37" + Content-Type: + - text/html + Date: + - Tue, 03 Jul 2018 01:38:53 GMT + Retry-After: + - "23" + Server: + - nginx + Vary: + - Authorization, X-Filter + X-Accepted-Oauth-Scopes: + - linodes:read_only + X-Frame-Options: + - DENY + X-Oauth-Scopes: + - '*' + X-Ratelimit-Limit: + - "400" + X-Ratelimit-Remaining: + - "391" + X-Ratelimit-Reset: + - "1530581957" + X-Spec-Version: + - 4.0.2 + status: 502 Bad Gateway + code: 502 + duration: "" From a65ff6dd3a61400261fbfae082176feb3467eb3a Mon Sep 17 00:00:00 2001 From: Adam Mohammed Date: Thu, 26 Sep 2019 09:54:07 -0400 Subject: [PATCH 2/3] Add test to check that bad content-type error bubbles up --- client_test.go | 18 +++++++++++++++++- errors.go | 6 +++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/client_test.go b/client_test.go index 8280c4389..37b0629cf 100644 --- a/client_test.go +++ b/client_test.go @@ -6,6 +6,7 @@ import ( "net/http" "os" "strconv" + "strings" "testing" "time" @@ -138,6 +139,21 @@ func TestClient_APIResponseBadGateway(t *testing.T) { t.Errorf("Expected Client to handle 502 from API Server") } }() - client.ListImages(context.Background(), nil) + + _, err := client.ListImages(context.Background(), nil) + + if err == nil { + t.Errorf("Error should be thrown on 502 Response from API") + } + + responseError, ok := err.(*Error) + + if !ok { + t.Errorf("Error type did not match the expected result") + } + + if !strings.Contains(responseError.Message, "Unexpected Content-Type") { + t.Errorf("Error message does not contain: \"Unexpected Content-Type\"") + } } diff --git a/errors.go b/errors.go index e0789d6cb..4f0fa0d1d 100644 --- a/errors.go +++ b/errors.go @@ -53,7 +53,11 @@ func coupleAPIErrors(r *resty.Response, err error) (*resty.Response, error) { expectedContentType := r.Request.Header.Get("Accept") responseContentType := r.Header().Get("Content-Type") if responseContentType != expectedContentType { - msg := fmt.Sprintf("Expected: %v; Received: %v", expectedContentType, responseContentType) + msg := fmt.Sprintf( + "Unexpected Content-Type: Expected: %v, Received: %v", + expectedContentType, + responseContentType, + ) return nil, NewError(msg) } From 308f806f6cd392ee1917121c3a24adbc59206ee8 Mon Sep 17 00:00:00 2001 From: Adam Mohammed Date: Thu, 26 Sep 2019 10:57:20 -0400 Subject: [PATCH 3/3] Fix url in fixture --- client_test.go | 2 +- fixtures/TestClient_APIResponseBadGateway.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client_test.go b/client_test.go index 37b0629cf..0820d048e 100644 --- a/client_test.go +++ b/client_test.go @@ -131,7 +131,7 @@ func TestClientAliases(t *testing.T) { } func TestClient_APIResponseBadGateway(t *testing.T) { - client, teardown := createTestClient(t, "fixtures/TestClient_APIResponseBadGateway.yaml") + client, teardown := createTestClient(t, "fixtures/TestClient_APIResponseBadGateway") defer teardown() defer func() { diff --git a/fixtures/TestClient_APIResponseBadGateway.yaml b/fixtures/TestClient_APIResponseBadGateway.yaml index 82c4cf0ce..1b75e1d49 100644 --- a/fixtures/TestClient_APIResponseBadGateway.yaml +++ b/fixtures/TestClient_APIResponseBadGateway.yaml @@ -13,7 +13,7 @@ interactions: - application/json User-Agent: - linodego 0.0.1 https://github.com/linode/linodego - url: https://api.linode.com/v4/linode/instances/1231 + url: https://api.linode.com/v4/images method: GET response: body: '