Repository: google/go-containerregistry
Pull request #639 (2019-12-17 18:18:43)

Improve transport.Error
Include http request in error message to provide context for what
failed.

Format multiple errors a bit better.

Add test coverage for when reading the response body fails.

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=h1) Report
> Merging [#639](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=desc) into [master](https://codecov.io/gh/google/go-containerregistry/commit/74b082017bc4c78230502fc932bd71970f3e1796?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/639/graphs/tree.svg?width=650&token=wG7iiDCdZ4&height=150&src=pr)](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master    #639      +/-   ##
=========================================
+ Coverage   76.53%   76.6%   +0.07%     
=========================================
  Files         101     101              
  Lines        4483    4489       +6     
=========================================
+ Hits         3431    3439       +8     
+ Misses        636     635       -1     
+ Partials      416     415       -1
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/transport/error.go](https://codecov.io/gh/google/go-containerregistry/pull/639/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvZXJyb3IuZ28=) | `100% <100%> (+5.26%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=footer). Last update [74b0820...8977a91](https://codecov.io/gh/google/go-containerregistry/pull/639?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/v1/remote/transport/error.go b/pkg/v1/remote/transport/error.go
index 35e5d798..a215e794 100644
--- a/pkg/v1/remote/transport/error.go
+++ b/pkg/v1/remote/transport/error.go
@@ -19,9 +19,27 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"strings"
 )
 
+// The set of query string keys that we expect to send as part of the registry
+// protocol. Anything else is potentially dangerous to leak, as it's probably
+// from a redirect. These redirects often included tokens or signed URLs.
+var paramWhitelist = map[string]struct{}{
+	// Token exchange
+	"scope":   struct{}{},
+	"service": struct{}{},
+	// Cross-repo mounting
+	"mount": struct{}{},
+	"from":  struct{}{},
+	// Layer PUT
+	"digest": struct{}{},
+	// Listing tags and catalog
+	"n":    struct{}{},
+	"last": struct{}{},
+}
+
 // Error implements error to support the following error specification:
 // https://github.com/docker/distribution/blob/master/docs/spec/api.md#errors
 type Error struct {
@@ -30,6 +48,8 @@ type Error struct {
 	StatusCode int
 	// The raw body if we couldn't understand it.
 	rawBody string
+	// The request that failed.
+	request *http.Request
 }
 
 // Check that Error implements error
@@ -37,6 +57,14 @@ var _ error = (*Error)(nil)
 
 // Error implements error
 func (e *Error) Error() string {
+	prefix := ""
+	if e.request != nil {
+		prefix = fmt.Sprintf("%s %s: ", e.request.Method, redact(e.request.URL))
+	}
+	return prefix + e.responseErr()
+}
+
+func (e *Error) responseErr() string {
 	switch len(e.Errors) {
 	case 0:
 		if len(e.rawBody) == 0 {
@@ -51,7 +79,7 @@ func (e *Error) Error() string {
 			errors = append(errors, d.String())
 		}
 		return fmt.Sprintf("multiple errors returned: %s",
-			strings.Join(errors, ";"))
+			strings.Join(errors, "; "))
 	}
 }
 
@@ -69,6 +97,21 @@ func (e *Error) Temporary() bool {
 	return true
 }
 
+func redact(original *url.URL) *url.URL {
+	qs := original.Query()
+	for k, v := range qs {
+		for i := range v {
+			if _, ok := paramWhitelist[k]; !ok {
+				// key is not in the whitelist
+				v[i] = "REDACTED"
+			}
+		}
+	}
+	redacted := *original
+	redacted.RawQuery = qs.Encode()
+	return &redacted
+}
+
 // Diagnostic represents a single error returned by a Docker registry interaction.
 type Diagnostic struct {
 	Code    ErrorCode   `json:"code"`
@@ -127,5 +170,6 @@ func CheckError(resp *http.Response, codes ...int) error {
 		structuredError.rawBody = string(b)
 	}
 	structuredError.StatusCode = resp.StatusCode
+	structuredError.request = resp.Request
 	return structuredError
 }


Test patch:
diff --git a/pkg/v1/remote/transport/error_test.go b/pkg/v1/remote/transport/error_test.go
index 4ca84708..85bd1279 100644
--- a/pkg/v1/remote/transport/error_test.go
+++ b/pkg/v1/remote/transport/error_test.go
@@ -17,8 +17,10 @@ package transport
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -79,9 +81,10 @@ func TestCheckErrorNil(t *testing.T) {
 
 func TestCheckErrorNotError(t *testing.T) {
 	tests := []struct {
-		code int
-		body string
-		msg  string
+		code    int
+		body    string
+		msg     string
+		request *http.Request
 	}{{
 		code: http.StatusBadRequest,
 		body: "",
@@ -89,13 +92,27 @@ func TestCheckErrorNotError(t *testing.T) {
 	}, {
 		code: http.StatusUnauthorized,
 		body: "Not JSON",
-		msg:  "unsupported status code 401; body: Not JSON",
+		msg:  "GET https://example.com/somepath?access_token=REDACTED&scope=foo&service=bar: unsupported status code 401; body: Not JSON",
+		request: &http.Request{
+			Method: http.MethodGet,
+			URL: &url.URL{
+				Scheme: "https",
+				Host:   "example.com",
+				Path:   "somepath",
+				RawQuery: url.Values{
+					"scope":        []string{"foo"},
+					"service":      []string{"bar"},
+					"access_token": []string{"hunter2"},
+				}.Encode(),
+			},
+		},
 	}}
 
 	for _, test := range tests {
 		resp := &http.Response{
 			StatusCode: test.code,
 			Body:       ioutil.NopCloser(bytes.NewBufferString(test.body)),
+			Request:    test.request,
 		}
 
 		err := CheckError(resp, http.StatusOK)
@@ -151,7 +168,7 @@ func TestCheckErrorWithError(t *testing.T) {
 			}},
 			StatusCode: 400,
 		},
-		msg: "multiple errors returned: NAME_INVALID: a message for you;SIZE_INVALID: another message for you; with some details",
+		msg: "multiple errors returned: NAME_INVALID: a message for you; SIZE_INVALID: another message for you; with some details",
 	}}
 
 	for _, test := range tests {
@@ -175,3 +192,28 @@ func TestCheckErrorWithError(t *testing.T) {
 		}
 	}
 }
+
+func TestBodyError(t *testing.T) {
+	expectedErr := errors.New("whoops")
+	resp := &http.Response{
+		StatusCode: http.StatusOK,
+		Body:       &errReadCloser{expectedErr},
+	}
+	if err := CheckError(resp, http.StatusNotFound); err == nil {
+		t.Errorf("CheckError() = nil, wanted error %v", expectedErr)
+	} else if err != expectedErr {
+		t.Errorf("CheckError() = %v, wanted %v", err, expectedErr)
+	}
+}
+
+type errReadCloser struct {
+	err error
+}
+
+func (e *errReadCloser) Read(p []byte) (int, error) {
+	return 0, e.err
+}
+
+func (e *errReadCloser) Close() error {
+	return e.err
+}