From 2ec0471bb5bb3e0380f5902ee38b92956fc56dff 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 (cherry picked from commit 45b7b9cec3a45cbf3343ce773fd5c88bc50f8a7f) Signed-off-by: Sebastiaan van Stijn --- registry/client/blob_writer_test.go | 2 ++ registry/client/errors.go | 51 ++++++++++++++++++++--------- registry/client/errors_test.go | 39 +++++++++++++++++----- registry/client/repository_test.go | 1 + 4 files changed, 68 insertions(+), 25 deletions(-) diff --git a/registry/client/blob_writer_test.go b/registry/client/blob_writer_test.go index f4469b6f5..be961a72e 100644 --- a/registry/client/blob_writer_test.go +++ b/registry/client/blob_writer_test.go @@ -85,6 +85,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": [ @@ -106,6 +107,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"), }, }, diff --git a/registry/client/errors.go b/registry/client/errors.go index 024df43dd..ce9902034 100644 --- a/registry/client/errors.go +++ b/registry/client/errors.go @@ -4,8 +4,8 @@ import ( "encoding/json" "errors" "fmt" - "io" "io/ioutil" + "mime" "net/http" "github.com/docker/distribution/registry/api/errcode" @@ -38,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 := ioutil.ReadAll(r) + body, err := ioutil.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 { @@ -52,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 { @@ -85,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) @@ -121,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 e142fc21b..0051500ba 100644 --- a/registry/client/repository_test.go +++ b/registry/client/repository_test.go @@ -1214,6 +1214,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"), }, })