]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go/internal/web: include snippets of plain-text server responses in error detail
authorBryan C. Mills <bcmills@google.com>
Mon, 5 Aug 2019 16:02:55 +0000 (12:02 -0400)
committerBryan C. Mills <bcmills@google.com>
Thu, 12 Sep 2019 14:09:20 +0000 (14:09 +0000)
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 <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
src/cmd/go/internal/web/api.go
src/cmd/go/internal/web/http.go
src/cmd/go/testdata/script/mod_auth.txt
src/cmd/go/testdata/script/mod_proxy_errors.txt [new file with mode: 0644]
src/cmd/go/testdata/script/mod_sumdb_file_path.txt

index cd0e19d3ffd1990226a3d41d3bfe66d8bbd18c61..ad99eb2f8c392633eb9d41bc5f86b70df2e9a64f 100644 (file)
 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
+}
index 757bcc8778f215c663a0671c38a3a93f68cb5a62..5e4319b00e9d5b794abf851b0f0c3acd9e4b53a8 100644 (file)
@@ -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
        }
 
index fe1d65794afb7205cbd23bfd048bb8d377ecf7a6..5bcbcd1a188f949690a03647f39147dc41f80031 100644 (file)
@@ -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 (file)
index 0000000..9cd1a82
--- /dev/null
@@ -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\?'
index 47c8a3a0f3cf654ee63375c8a88b1565a3ae073e..4f4b99575a5e6809eb7b46858a3f1b79572280e6 100644 (file)
@@ -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,