From 896443d63bdc23ab8a255c0d0157ee5ce2f5ae4e Mon Sep 17 00:00:00 2001 From: Isaac Good Date: Thu, 12 Jun 2025 23:12:05 -0700 Subject: [PATCH 1/2] Check HTTP response content type before trying to decode it as JSON --- cmd/cmd.go | 7 +++++++ cmd/cmd_test.go | 47 ++++++++++++++++++++++++++++++++++++++++++++ cmd/download_test.go | 2 +- cmd/submit_test.go | 2 +- 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index dcca08264..57b579f6b 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -77,6 +77,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"); contentType != "application/json" { + 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"` diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index 782be18d2..0bdae9e80 100644 --- a/cmd/cmd_test.go +++ b/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,47 @@ 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/json", `{"error": {}}`), + wantMessage: "unexpected API response: 418", + }, + } + for _, tc := range testCases { + got := decodedAPIError(tc.response) + assert.Equal(t, tc.wantMessage, got.Error()) + } +} diff --git a/cmd/download_test.go b/cmd/download_test.go index aadcf84aa..791be4230 100644 --- a/cmd/download_test.go +++ b/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()) } diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 9b645965b..7db152a75 100644 --- a/cmd/submit_test.go +++ b/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) { From df4b5a853bb019f629b43876cca2aba7123636a2 Mon Sep 17 00:00:00 2001 From: Isaac Good Date: Fri, 13 Jun 2025 08:52:28 -0700 Subject: [PATCH 2/2] Extend the content type matching to include `application/foo+json` format --- cmd/cmd.go | 5 ++++- cmd/cmd_test.go | 9 ++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index 57b579f6b..91597b5b1 100644 --- a/cmd/cmd.go +++ b/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,7 +80,7 @@ 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"); contentType != "application/json" { + 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, diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index 0bdae9e80..101c52c87 100644 --- a/cmd/cmd_test.go +++ b/cmd/cmd_test.go @@ -156,12 +156,19 @@ func TestDecodeErrorResponse(t *testing.T) { 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", }, } - for _, tc := range testCases { + 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()) }