From 45b7b9cec3a45cbf3343ce773fd5c88bc50f8a7f Mon Sep 17 00:00:00 2001 From: Milos Gajdos Date: Thu, 24 Aug 2023 08:08:04 +0100 Subject: [PATCH] Dont parse errors as JSON unless Content-Type is set to JSON Client attempts to parse the body of every error it receives as JSON regardless of the content-type. This commit rectifies by only parsing he error body as JSON if the Content-Type header is set to either "application/json" or "application/vnd.api+json". Signed-off-by: Milos Gajdos --- registry/client/blob_writer_test.go | 4 +++ registry/client/errors.go | 50 ++++++++++++++++++++--------- registry/client/errors_test.go | 39 ++++++++++++++++------ registry/client/repository_test.go | 1 + 4 files changed, 70 insertions(+), 24 deletions(-) diff --git a/registry/client/blob_writer_test.go b/registry/client/blob_writer_test.go index 0eb0b39b4..d9947aaed 100644 --- a/registry/client/blob_writer_test.go +++ b/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"), }, }, diff --git a/registry/client/errors.go b/registry/client/errors.go index 8394ee0c2..30c769261 100644 --- a/registry/client/errors.go +++ b/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 { - 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) - } + return makeError(statusCode, 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.StatusCode, resp.Body)) + return mergeErrors(err, parseHTTPErrorResponse(resp)) } } - err := parseHTTPErrorResponse(resp.StatusCode, resp.Body) + err := parseHTTPErrorResponse(resp) if uErr, ok := err.(*UnexpectedHTTPResponseError); ok && resp.StatusCode == 401 { return errcode.ErrorCodeUnauthorized.WithDetail(uErr.Response) } diff --git a/registry/client/errors_test.go b/registry/client/errors_test.go index 6be399d03..135608c77 100644 --- a/registry/client/errors_test.go +++ b/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()) } } diff --git a/registry/client/repository_test.go b/registry/client/repository_test.go index 838c1621d..56681c0fd 100644 --- a/registry/client/repository_test.go +++ b/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("garbage"), }, })