Merge pull request #4013 from milosgajdos/nonjson-error-client

Dont parse errors as JSON unless Content-Type is set to JSON

registry/client/blob_writer_test.go

@@ -86,6 +86,7 @@ func TestUploadReadFrom(t *testing.T) {
 			},
 			Response: testutil.Response{
 				StatusCode: http.StatusBadRequest,
+				Headers:    http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 				Body: []byte(`
 					{ "errors":
 						[
@@ -107,6 +108,7 @@ func TestUploadReadFrom(t *testing.T) {
 			},
 			Response: testutil.Response{
 				StatusCode: http.StatusBadRequest,
+				Headers:    http.Header{"Content-Type": []string{"application/json"}},
 				Body:       []byte("something bad happened"),
 			},
 		},
@@ -372,6 +374,7 @@ func TestUploadWrite(t *testing.T) {
 			},
 			Response: testutil.Response{
 				StatusCode: http.StatusBadRequest,
+				Headers:    http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 				Body: []byte(`
 					{ "errors":
 						[
@@ -393,6 +396,7 @@ func TestUploadWrite(t *testing.T) {
 			},
 			Response: testutil.Response{
 				StatusCode: http.StatusBadRequest,
+				Headers:    http.Header{"Content-Type": []string{"application/json"}},
 				Body:       []byte("something bad happened"),
 			},
 		},

registry/client/errors.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"mime"
 	"net/http"
 
 	"github.com/distribution/distribution/v3/registry/api/errcode"
@@ -37,13 +38,29 @@ func (e *UnexpectedHTTPResponseError) Error() string {
 	return fmt.Sprintf("error parsing HTTP %d response body: %s: %q", e.StatusCode, e.ParseErr.Error(), string(e.Response))
 }
 
-func parseHTTPErrorResponse(statusCode int, r io.Reader) error {
+func parseHTTPErrorResponse(resp *http.Response) error {
 	var errors errcode.Errors
-	body, err := io.ReadAll(r)
+	body, err := io.ReadAll(resp.Body)
 	if err != nil {
 		return err
 	}
 
+	statusCode := resp.StatusCode
+	ctHeader := resp.Header.Get("Content-Type")
+
+	if ctHeader == "" {
+		return makeError(statusCode, string(body))
+	}
+
+	contentType, _, err := mime.ParseMediaType(ctHeader)
+	if err != nil {
+		return fmt.Errorf("failed parsing content-type: %w", err)
+	}
+
+	if contentType != "application/json" && contentType != "application/vnd.api+json" {
+		return makeError(statusCode, string(body))
+	}
+
 	// For backward compatibility, handle irregularly formatted
 	// messages that contain a "details" field.
 	var detailsErr struct {
@@ -51,16 +68,7 @@ func parseHTTPErrorResponse(statusCode int, r io.Reader) error {
 	}
 	err = json.Unmarshal(body, &detailsErr)
 	if err == nil && detailsErr.Details != "" {
-		switch statusCode {
+		return makeError(statusCode, detailsErr.Details)
-		case http.StatusUnauthorized:
-			return errcode.ErrorCodeUnauthorized.WithMessage(detailsErr.Details)
-		case http.StatusForbidden:
-			return errcode.ErrorCodeDenied.WithMessage(detailsErr.Details)
-		case http.StatusTooManyRequests:
-			return errcode.ErrorCodeTooManyRequests.WithMessage(detailsErr.Details)
-		default:
-			return errcode.ErrorCodeUnknown.WithMessage(detailsErr.Details)
-		}
 	}
 
 	if err := json.Unmarshal(body, &errors); err != nil {
@@ -84,6 +92,19 @@ func parseHTTPErrorResponse(statusCode int, r io.Reader) error {
 	return errors
 }
 
+func makeError(statusCode int, details string) error {
+	switch statusCode {
+	case http.StatusUnauthorized:
+		return errcode.ErrorCodeUnauthorized.WithMessage(details)
+	case http.StatusForbidden:
+		return errcode.ErrorCodeDenied.WithMessage(details)
+	case http.StatusTooManyRequests:
+		return errcode.ErrorCodeTooManyRequests.WithMessage(details)
+	default:
+		return errcode.ErrorCodeUnknown.WithMessage(details)
+	}
+}
+
 func makeErrorList(err error) []error {
 	if errL, ok := err.(errcode.Errors); ok {
 		return []error(errL)
@@ -120,11 +141,10 @@ func HandleErrorResponse(resp *http.Response) error {
 				} else {
 					err.Message = err.Code.Message()
 				}
-
+				return mergeErrors(err, parseHTTPErrorResponse(resp))
-				return mergeErrors(err, parseHTTPErrorResponse(resp.StatusCode, resp.Body))
 			}
 		}
-		err := parseHTTPErrorResponse(resp.StatusCode, resp.Body)
+		err := parseHTTPErrorResponse(resp)
 		if uErr, ok := err.(*UnexpectedHTTPResponseError); ok && resp.StatusCode == 401 {
 			return errcode.ErrorCodeUnauthorized.WithDetail(uErr.Response)
 		}

registry/client/errors_test.go

@@ -15,17 +15,18 @@ type nopCloser struct {
 func (nopCloser) Close() error { return nil }
 
 func TestHandleErrorResponse401ValidBody(t *testing.T) {
-	json := "{\"errors\":[{\"code\":\"UNAUTHORIZED\",\"message\":\"action requires authentication\"}]}"
+	json := `{"errors":[{"code":"UNAUTHORIZED","message":"action requires authentication"}]}`
 	response := &http.Response{
 		Status:     "401 Unauthorized",
 		StatusCode: 401,
 		Body:       nopCloser{bytes.NewBufferString(json)},
+		Header:     http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := "unauthorized: action requires authentication"
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
 	}
 }
 
@@ -35,27 +36,29 @@ func TestHandleErrorResponse401WithInvalidBody(t *testing.T) {
 		Status:     "401 Unauthorized",
 		StatusCode: 401,
 		Body:       nopCloser{bytes.NewBufferString(json)},
+		Header:     http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := "unauthorized: authentication required"
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
 	}
 }
 
 func TestHandleErrorResponseExpectedStatusCode400ValidBody(t *testing.T) {
-	json := "{\"errors\":[{\"code\":\"DIGEST_INVALID\",\"message\":\"provided digest does not match\"}]}"
+	json := `{"errors":[{"code":"DIGEST_INVALID","message":"provided digest does not match"}]}`
 	response := &http.Response{
 		Status:     "400 Bad Request",
 		StatusCode: 400,
 		Body:       nopCloser{bytes.NewBufferString(json)},
+		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := "digest invalid: provided digest does not match"
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
 	}
 }
 
@@ -65,12 +68,13 @@ func TestHandleErrorResponseExpectedStatusCode404EmptyErrorSlice(t *testing.T) {
 		Status:     "404 Not Found",
 		StatusCode: 404,
 		Body:       nopCloser{bytes.NewBufferString(json)},
+		Header:     http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := `error parsing HTTP 404 response body: no error details found in HTTP response body: "{\"randomkey\": \"randomvalue\"}"`
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
 	}
 }
 
@@ -80,12 +84,13 @@ func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) {
 		Status:     "404 Not Found",
 		StatusCode: 404,
 		Body:       nopCloser{bytes.NewBufferString(json)},
+		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := "error parsing HTTP 404 response body: invalid character 'i' looking for beginning of object key string: \"{invalid json}\""
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
 	}
 }
 
@@ -94,12 +99,13 @@ func TestHandleErrorResponseUnexpectedStatusCode501(t *testing.T) {
 		Status:     "501 Not Implemented",
 		StatusCode: 501,
 		Body:       nopCloser{bytes.NewBufferString("{\"Error Encountered\" : \"Function not implemented.\"}")},
+		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := "received unexpected HTTP status: 501 Not Implemented"
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
 	}
 }
 
@@ -109,11 +115,26 @@ func TestHandleErrorResponseInsufficientPrivileges403(t *testing.T) {
 		Status:     "403 Forbidden",
 		StatusCode: 403,
 		Body:       nopCloser{bytes.NewBufferString(json)},
+		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
 	err := HandleErrorResponse(response)
 
 	expectedMsg := "denied: requesting higher privileges than access token allows"
 	if !strings.Contains(err.Error(), expectedMsg) {
-		t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error())
+		t.Errorf("Expected %q, got: %q", expectedMsg, err.Error())
+	}
+}
+
+func TestHandleErrorResponseNonJson(t *testing.T) {
+	msg := `{"details":"requesting higher privileges than access token allows"}`
+	response := &http.Response{
+		Status:     "403 Forbidden",
+		StatusCode: 403,
+		Body:       nopCloser{bytes.NewBufferString(msg)},
+	}
+	err := HandleErrorResponse(response)
+
+	if !strings.Contains(err.Error(), msg) {
+		t.Errorf("Expected %q, got: %q", msg, err.Error())
 	}
 }

registry/client/repository_test.go

@@ -1603,6 +1603,7 @@ func TestManifestUnauthorized(t *testing.T) {
 		},
 		Response: testutil.Response{
 			StatusCode: http.StatusUnauthorized,
+			Headers:    http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 			Body:       []byte("<html>garbage</html>"),
 		},
 	})