Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Aug 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions registry/client/blob_writer_test.go
View file
Open in desktop
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
Open in desktop
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" {
thaJeztah marked this conversation as resolved.
Show resolved Hide resolved
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we have to check for if len(body) == 0 as well. This is relevant for HEAD requests for example. I'm currently getting

time="2024-03-20T08:19:56.45287383Z" level=error msg="response completed with error" err.code=unknown err.detail="errors:
denied: requested access to the resource is denied
error parsing HTTP 401 response body: unexpected end of JSON input: ""
" err.message="unknown error" go.version=go1.20.8 http.request.host="<redacted>" http.request.id=8a2db3dd-a021-4864-8a56-307617a3cf26 http.request.method=HEAD http.request.remoteaddr="<redacted>" http.request.uri="/v2/<redacted>/<redacted>/blobs/sha256:<redacted>?ns=<redacted>" http.request.useragent="containerd/v1.6.28" http.response.contenttype="application/json; charset=utf-8" http.response.duration=213.502875ms http.response.status=500 http.response.written=264 vars.digest="sha256:<redacted>" vars.name="<redacted>"

in proxy mode against a non-existing image.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. yes, looks like we may need to? (at least I don't think it hurts?). Wondering if in that case it should produce a generic error (based on status code)?

Contributions / PR welcome!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess that last part may already be handled, as this is only for "details", so maybe that's not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go: #4307.

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
Open in desktop
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
Open in desktop
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