From 9a44023ce922407fe4e847400924e2b2980bd2c5 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Mon, 5 Aug 2019 12:02:55 -0400 Subject: [PATCH] cmd/go/internal/web: include snippets of plain-text server responses in error detail For the server response to be displayed, the response must be served as type text/plain with charset us-ascii or utf-8, and must consist of only graphic characters and whitespace. We truncate the server response at the first blank line or after 8 lines or a fixed number of characters, and tab-indent (if multiple lines) to ensure that the response is offset from ordinary go command output. Fixes #30748 Change-Id: I0bc1d734737e456e3251aee2252463b6355e8c97 Reviewed-on: https://go-review.googlesource.com/c/go/+/189783 Run-TryBot: Bryan C. Mills TryBot-Result: Gobot Gobot Reviewed-by: Jay Conrod --- src/cmd/go/internal/web/api.go | 125 +++++++++++++++++- src/cmd/go/internal/web/http.go | 21 ++- src/cmd/go/testdata/script/mod_auth.txt | 2 + .../go/testdata/script/mod_proxy_errors.txt | 19 +++ .../testdata/script/mod_sumdb_file_path.txt | 5 +- 5 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_proxy_errors.txt diff --git a/src/cmd/go/internal/web/api.go b/src/cmd/go/internal/web/api.go index cd0e19d3ffd19..ad99eb2f8c392 100644 --- a/src/cmd/go/internal/web/api.go +++ b/src/cmd/go/internal/web/api.go @@ -10,12 +10,15 @@ package web import ( + "bytes" "fmt" "io" "io/ioutil" "net/url" "os" "strings" + "unicode" + "unicode/utf8" ) // SecurityMode specifies whether a function should make network @@ -34,9 +37,32 @@ type HTTPError struct { URL string // redacted Status string StatusCode int + Err error // underlying error, if known + Detail string // limited to maxErrorDetailLines and maxErrorDetailBytes } +const ( + maxErrorDetailLines = 8 + maxErrorDetailBytes = maxErrorDetailLines * 81 +) + func (e *HTTPError) Error() string { + if e.Detail != "" { + detailSep := " " + if strings.ContainsRune(e.Detail, '\n') { + detailSep = "\n\t" + } + return fmt.Sprintf("reading %s: %v\n\tserver response:%s%s", e.URL, e.Status, detailSep, e.Detail) + } + + if err := e.Err; err != nil { + if pErr, ok := e.Err.(*os.PathError); ok && strings.HasSuffix(e.URL, pErr.Path) { + // Remove the redundant copy of the path. + err = pErr.Err + } + return fmt.Sprintf("reading %s: %v", e.URL, err) + } + return fmt.Sprintf("reading %s: %v", e.URL, e.Status) } @@ -44,6 +70,10 @@ func (e *HTTPError) Is(target error) bool { return target == os.ErrNotExist && (e.StatusCode == 404 || e.StatusCode == 410) } +func (e *HTTPError) Unwrap() error { + return e.Err +} + // GetBytes returns the body of the requested resource, or an error if the // response status was not http.StatusOK. // @@ -69,16 +99,69 @@ type Response struct { Status string StatusCode int Header map[string][]string - Body io.ReadCloser + Body io.ReadCloser // Either the original body or &errorDetail. + + fileErr error + errorDetail errorDetailBuffer } // Err returns an *HTTPError corresponding to the response r. -// It returns nil if the response r has StatusCode 200 or 0 (unset). +// If the response r has StatusCode 200 or 0 (unset), Err returns nil. +// Otherwise, Err may read from r.Body in order to extract relevant error detail. func (r *Response) Err() error { if r.StatusCode == 200 || r.StatusCode == 0 { return nil } - return &HTTPError{URL: r.URL, Status: r.Status, StatusCode: r.StatusCode} + + return &HTTPError{ + URL: r.URL, + Status: r.Status, + StatusCode: r.StatusCode, + Err: r.fileErr, + Detail: r.formatErrorDetail(), + } +} + +// formatErrorDetail converts r.errorDetail (a prefix of the output of r.Body) +// into a short, tab-indented summary. +func (r *Response) formatErrorDetail() string { + if r.Body != &r.errorDetail { + return "" // Error detail collection not enabled. + } + + // Ensure that r.errorDetail has been populated. + _, _ = io.Copy(ioutil.Discard, r.Body) + + s := r.errorDetail.buf.String() + if !utf8.ValidString(s) { + return "" // Don't try to recover non-UTF-8 error messages. + } + for _, r := range s { + if !unicode.IsGraphic(r) && !unicode.IsSpace(r) { + return "" // Don't let the server do any funny business with the user's terminal. + } + } + + var detail strings.Builder + for i, line := range strings.Split(s, "\n") { + if strings.TrimSpace(line) == "" { + break // Stop at the first blank line. + } + if i > 0 { + detail.WriteString("\n\t") + } + if i >= maxErrorDetailLines { + detail.WriteString("[Truncated: too many lines.]") + break + } + if detail.Len()+len(line) > maxErrorDetailBytes { + detail.WriteString("[Truncated: too long.]") + break + } + detail.WriteString(line) + } + + return detail.String() } // Get returns the body of the HTTP or HTTPS resource specified at the given URL. @@ -131,3 +214,39 @@ func Join(u *url.URL, path string) *url.URL { j.RawPath = strings.TrimSuffix(u.RawPath, "/") + "/" + strings.TrimPrefix(path, "/") return &j } + +// An errorDetailBuffer is an io.ReadCloser that copies up to +// maxErrorDetailLines into a buffer for later inspection. +type errorDetailBuffer struct { + r io.ReadCloser + buf strings.Builder + bufLines int +} + +func (b *errorDetailBuffer) Close() error { + return b.r.Close() +} + +func (b *errorDetailBuffer) Read(p []byte) (n int, err error) { + n, err = b.r.Read(p) + + // Copy the first maxErrorDetailLines+1 lines into b.buf, + // discarding any further lines. + // + // Note that the read may begin or end in the middle of a UTF-8 character, + // so don't try to do anything fancy with characters that encode to larger + // than one byte. + if b.bufLines <= maxErrorDetailLines { + for _, line := range bytes.SplitAfterN(p[:n], []byte("\n"), maxErrorDetailLines-b.bufLines) { + b.buf.Write(line) + if len(line) > 0 && line[len(line)-1] == '\n' { + b.bufLines++ + if b.bufLines > maxErrorDetailLines { + break + } + } + } + } + + return n, err +} diff --git a/src/cmd/go/internal/web/http.go b/src/cmd/go/internal/web/http.go index 757bcc8778f21..5e4319b00e9d5 100644 --- a/src/cmd/go/internal/web/http.go +++ b/src/cmd/go/internal/web/http.go @@ -14,7 +14,7 @@ package web import ( "crypto/tls" "fmt" - "io/ioutil" + "mime" "net/http" urlpkg "net/url" "os" @@ -64,7 +64,7 @@ func get(security SecurityMode, url *urlpkg.URL) (*Response, error) { Status: "404 testing", StatusCode: 404, Header: make(map[string][]string), - Body: ioutil.NopCloser(strings.NewReader("")), + Body: http.NoBody, } if cfg.BuildX { fmt.Fprintf(os.Stderr, "# get %s: %v (%.3fs)\n", Redacted(url), res.Status, time.Since(start).Seconds()) @@ -167,6 +167,7 @@ func get(security SecurityMode, url *urlpkg.URL) (*Response, error) { if cfg.BuildX { fmt.Fprintf(os.Stderr, "# get %s: %v (%.3fs)\n", Redacted(fetched), res.Status, time.Since(start).Seconds()) } + r := &Response{ URL: Redacted(fetched), Status: res.Status, @@ -174,6 +175,20 @@ func get(security SecurityMode, url *urlpkg.URL) (*Response, error) { Header: map[string][]string(res.Header), Body: res.Body, } + + if res.StatusCode != http.StatusOK { + contentType := res.Header.Get("Content-Type") + if mediaType, params, _ := mime.ParseMediaType(contentType); mediaType == "text/plain" { + switch charset := strings.ToLower(params["charset"]); charset { + case "us-ascii", "utf-8", "": + // Body claims to be plain text in UTF-8 or a subset thereof. + // Try to extract a useful error message from it. + r.errorDetail.r = res.Body + r.Body = &r.errorDetail + } + } + } + return r, nil } @@ -190,6 +205,7 @@ func getFile(u *urlpkg.URL) (*Response, error) { Status: http.StatusText(http.StatusNotFound), StatusCode: http.StatusNotFound, Body: http.NoBody, + fileErr: err, }, nil } @@ -199,6 +215,7 @@ func getFile(u *urlpkg.URL) (*Response, error) { Status: http.StatusText(http.StatusForbidden), StatusCode: http.StatusForbidden, Body: http.NoBody, + fileErr: err, }, nil } diff --git a/src/cmd/go/testdata/script/mod_auth.txt b/src/cmd/go/testdata/script/mod_auth.txt index fe1d65794afb7..5bcbcd1a188f9 100644 --- a/src/cmd/go/testdata/script/mod_auth.txt +++ b/src/cmd/go/testdata/script/mod_auth.txt @@ -8,6 +8,8 @@ env GOSUMDB=off # basic auth should fail. env NETRC=$WORK/empty ! go list all +stderr '^\tserver response: ACCESS DENIED, buddy$' +stderr '^\tserver response: File\? What file\?$' # With credentials from a netrc file, it should succeed. env NETRC=$WORK/netrc diff --git a/src/cmd/go/testdata/script/mod_proxy_errors.txt b/src/cmd/go/testdata/script/mod_proxy_errors.txt new file mode 100644 index 0000000000000..9cd1a824f0848 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_proxy_errors.txt @@ -0,0 +1,19 @@ +[!net] skip + +env GO111MODULE=on +env GOSUMDB=off +env GOPROXY=direct + +# Server responses should be truncated to some reasonable number of lines. +# (For now, exactly eight.) +! go list -m vcs-test.golang.org/auth/ormanylines@latest +stderr '\tserver response:\n(.|\n)*\tline 8\n\t\[Truncated: too many lines.\]$' + +# Server responses should be truncated to some reasonable number of characters. +! go list -m vcs-test.golang.org/auth/oronelongline@latest +! stderr 'blah{40}' +stderr '\tserver response: \[Truncated: too long\.\]$' + +# Responses from servers using the 'mod' protocol should be propagated. +! go list -m vcs-test.golang.org/go/modauth404@latest +stderr '\tserver response: File\? What file\?' diff --git a/src/cmd/go/testdata/script/mod_sumdb_file_path.txt b/src/cmd/go/testdata/script/mod_sumdb_file_path.txt index 47c8a3a0f3cf6..4f4b99575a5e6 100644 --- a/src/cmd/go/testdata/script/mod_sumdb_file_path.txt +++ b/src/cmd/go/testdata/script/mod_sumdb_file_path.txt @@ -7,10 +7,13 @@ env GOPATH=$WORK/gopath1 # With a file-based proxy with an empty checksum directory, # downloading a new module should fail, even if a subsequent # proxy contains a more complete mirror of the sum database. +# +# TODO(bcmills): The error message here is a bit redundant. +# It comes from the sumweb package, which isn't yet producing structured errors. [windows] env GOPROXY=file:///$WORK/sumproxy,https://proxy.golang.org [!windows] env GOPROXY=file://$WORK/sumproxy,https://proxy.golang.org ! go get -d golang.org/x/text@v0.3.2 -stderr '^verifying golang.org/x/text.*: Not Found' +stderr '^verifying golang.org/x/text@v0.3.2: golang.org/x/text@v0.3.2: reading file://.*/sumdb/sum.golang.org/lookup/golang.org/x/text@v0.3.2: (no such file or directory|.*cannot find the file specified.*)' # If the proxy does not claim to support the database, # checksum verification should fall through to the next proxy,