Check HTTP response content type before trying to decode it as JSON (#1196)

Author: IsaacG

cmd/cmd.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"regexp"
 	"strings"
 
 	"io"
@@ -23,6 +24,8 @@ var (
 	Out io.Writer
 	// Err is used to write errors.
 	Err io.Writer
+	// jsonContentTypeRe is used to match Content-Type which contains JSON.
+	jsonContentTypeRe = regexp.MustCompile(`^application/([[:alpha:]]+\+)?json$`)
 )
 
 const msgWelcomePleaseConfigure = `
@@ -77,6 +80,13 @@ func validateUserConfig(cfg *viper.Viper) error {
 // decodedAPIError decodes and returns the error message from the API response.
 // If the message is blank, it returns a fallback message with the status code.
 func decodedAPIError(resp *http.Response) error {
+	if contentType := resp.Header.Get("Content-Type"); !jsonContentTypeRe.MatchString(contentType) {
+		return fmt.Errorf(
+			"expected response with Content-Type \"application/json\" but got status %q with Content-Type %q",
+			resp.Status,
+			contentType,
+		)
+	}
 	var apiError struct {
 		Error struct {
 			Type             string   `json:"type"`

cmd/cmd_test.go

@@ -2,7 +2,10 @@ package cmd
 
 import (
 	"io"
+	"io/ioutil"
+	"net/http"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -119,3 +122,54 @@ func (co capturedOutput) reset() {
 	Out = co.oldOut
 	Err = co.oldErr
 }
+
+func errorResponse(contentType string, body string) *http.Response {
+	response := &http.Response{
+		Status:        "418 I'm a teapot",
+		StatusCode:    418,
+		Header:        make(http.Header),
+		Body:          ioutil.NopCloser(strings.NewReader(body)),
+		ContentLength: int64(len(body)),
+	}
+	response.Header.Set("Content-Type", contentType)
+	return response
+}
+
+func TestDecodeErrorResponse(t *testing.T) {
+	testCases := []struct {
+		response    *http.Response
+		wantMessage string
+	}{
+		{
+			response:    errorResponse("text/html", "Time for tea"),
+			wantMessage: `expected response with Content-Type "application/json" but got status "418 I'm a teapot" with Content-Type "text/html"`,
+		},
+		{
+			response:    errorResponse("application/json", `{"error": {"type": "json", "valid": no}}`),
+			wantMessage: "failed to parse API error response: invalid character 'o' in literal null (expecting 'u')",
+		},
+		{
+			response:    errorResponse("application/json", `{"error": {"type": "track_ambiguous", "message": "message", "possible_track_ids": ["a", "b"]}}`),
+			wantMessage: "message: a, b",
+		},
+		{
+			response:    errorResponse("application/json", `{"error": {"message": "message"}}`),
+			wantMessage: "message",
+		},
+		{
+			response:    errorResponse("application/problem+json", `{"error": {"message": "new json format"}}`),
+			wantMessage: "new json format",
+		},
+		{
+			response:    errorResponse("application/json", `{"error": {}}`),
+			wantMessage: "unexpected API response: 418",
+		},
+	}
+	tc := testCases[0]
+	got := decodedAPIError(tc.response)
+	assert.Equal(t, tc.wantMessage, got.Error())
+	for _, tc = range testCases {
+		got := decodedAPIError(tc.response)
+		assert.Equal(t, tc.wantMessage, got.Error())
+	}
+}

cmd/download_test.go

@@ -403,7 +403,7 @@ func TestDownloadError(t *testing.T) {
 
 	err = runDownload(cfg, flags, []string{})
 
-	assert.Equal(t, "test error", err.Error())
+	assert.Equal(t, `expected response with Content-Type "application/json" but got status "400 Bad Request" with Content-Type "text/plain; charset=utf-8"`, err.Error())
 
 }
 

cmd/submit_test.go

@@ -657,7 +657,7 @@ func TestSubmitServerErr(t *testing.T) {
 
 	err = runSubmit(cfg, pflag.NewFlagSet("fake", pflag.PanicOnError), files)
 
-	assert.Regexp(t, "test error", err.Error())
+	assert.Regexp(t, `expected response with Content-Type "application/json" but got status "400 Bad Request" with Content-Type "text/plain; charset=utf-8"`, err.Error())
 }
 
 func TestHandleErrorResponse(t *testing.T) {