Skip to content

Commit

Permalink
Dont parse errors as JSON unless Content-Type is set to JSON
Browse files Browse the repository at this point in the history 
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".

Co-authored-by: Sebastiaan van Stijn <thaJeztah@users.noreply.github.com>

Signed-off-by: Milos Gajdos <milosgajdos83@gmail.com>
  • Loading branch information
@milosgajdos
milosgajdos committed Aug 28, 2023
1 parent 4f7424c commit 886425d
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 24 deletions.
4 changes: 4 additions & 0 deletions registry/client/blob_writer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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":
[
Expand All @@ -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"),
},
},
Expand Down Expand Up @@ -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":
[
Expand All @@ -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"),
},
},
Expand Down
50 changes: 35 additions & 15 deletions registry/client/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"io"
"mime"
"net/http"

"github.com/distribution/distribution/v3/registry/api/errcode"
Expand Down Expand Up @@ -37,30 +38,37 @@ 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 {
Details string `json:"details"`
}
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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
39 changes: 30 additions & 9 deletions registry/client/errors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}

Expand All @@ -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())
}
}

Expand All @@ -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())
}
}

Expand All @@ -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())
}
}

Expand All @@ -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())
}
}

Expand All @@ -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())
}
}
1 change: 1 addition & 0 deletions registry/client/repository_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>"),
},
})
Expand Down

0 comments on commit 886425d

Please sign in to comment.