From: Dmitri Shuralyov Date: Sun, 1 Oct 2023 21:17:44 +0000 (-0400) Subject: cmd/go/internal/modfetch: show real URL in response body read errors X-Git-Tag: go1.22rc1~433 X-Git-Url: http://www.git.cypherpunks.ru/?a=commitdiff_plain;h=8b14e998fbd09512a1be96361e62726ca90552f7;p=gostls13.git cmd/go/internal/modfetch: show real URL in response body read errors CL 233437 added a redactedURL field to proxyRepo, a struct that already had a field named 'url'. Neither fields were documented, so the similar names suggest the most natural interpretation that proxyRepo.redactedURL is equivalent to proxyRepo.url.Redacted() rather than something else. That's possibly why it was joined with the module version in CL 406675. It turns out the two URLs differ in more than just redaction: one is the base proxy URL with (escaped) module path joined, the other is just the base proxy URL, in redacted form. Document and rename the fields to make the distinction more clear, and include all 3 of base module proxy URL + module path + module version in the reported URL, rather than just the first and third bits as seen in the errors at https://go.dev/issue/51323#issuecomment-1735812250. For #51323. Updates #38680. Updates #52727. Change-Id: Ib4b134b548adeec826ee88fe51a2cf580fde0516 Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/532035 LUCI-TryBot-Result: Go LUCI Reviewed-by: Bryan Mills Auto-Submit: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov --- diff --git a/src/cmd/go/internal/modfetch/proxy.go b/src/cmd/go/internal/modfetch/proxy.go index 56a6aaa40d..e0efb097ec 100644 --- a/src/cmd/go/internal/modfetch/proxy.go +++ b/src/cmd/go/internal/modfetch/proxy.go @@ -185,9 +185,9 @@ func TryProxies(f func(proxy string) error) error { } type proxyRepo struct { - url *url.URL - path string - redactedURL string + url *url.URL // The combined module proxy URL joined with the module path. + path string // The module path (unescaped). + redactedBase string // The base module proxy URL in [url.URL.Redacted] form. listLatestOnce sync.Once listLatest *RevInfo @@ -195,31 +195,35 @@ type proxyRepo struct { } func newProxyRepo(baseURL, path string) (Repo, error) { + // Parse the base proxy URL. base, err := url.Parse(baseURL) if err != nil { return nil, err } + redactedBase := base.Redacted() switch base.Scheme { case "http", "https": // ok case "file": if *base != (url.URL{Scheme: base.Scheme, Path: base.Path, RawPath: base.RawPath}) { - return nil, fmt.Errorf("invalid file:// proxy URL with non-path elements: %s", base.Redacted()) + return nil, fmt.Errorf("invalid file:// proxy URL with non-path elements: %s", redactedBase) } case "": - return nil, fmt.Errorf("invalid proxy URL missing scheme: %s", base.Redacted()) + return nil, fmt.Errorf("invalid proxy URL missing scheme: %s", redactedBase) default: - return nil, fmt.Errorf("invalid proxy URL scheme (must be https, http, file): %s", base.Redacted()) + return nil, fmt.Errorf("invalid proxy URL scheme (must be https, http, file): %s", redactedBase) } + // Append the module path to the URL. + url := base enc, err := module.EscapePath(path) if err != nil { return nil, err } - redactedURL := base.Redacted() - base.Path = strings.TrimSuffix(base.Path, "/") + "/" + enc - base.RawPath = strings.TrimSuffix(base.RawPath, "/") + "/" + pathEscape(enc) - return &proxyRepo{base, path, redactedURL, sync.Once{}, nil, nil}, nil + url.Path = strings.TrimSuffix(base.Path, "/") + "/" + enc + url.RawPath = strings.TrimSuffix(base.RawPath, "/") + "/" + pathEscape(enc) + + return &proxyRepo{url, path, redactedBase, sync.Once{}, nil, nil}, nil } func (p *proxyRepo) ModulePath() string { @@ -253,7 +257,7 @@ func (p *proxyRepo) versionError(version string, err error) error { } func (p *proxyRepo) getBytes(ctx context.Context, path string) ([]byte, error) { - body, err := p.getBody(ctx, path) + body, redactedURL, err := p.getBody(ctx, path) if err != nil { return nil, err } @@ -261,14 +265,14 @@ func (p *proxyRepo) getBytes(ctx context.Context, path string) ([]byte, error) { b, err := io.ReadAll(body) if err != nil { - // net/http doesn't add context to Body errors, so add it here. + // net/http doesn't add context to Body read errors, so add it here. // (See https://go.dev/issue/52727.) - return b, &url.Error{Op: "read", URL: strings.TrimSuffix(p.redactedURL, "/") + "/" + path, Err: err} + return b, &url.Error{Op: "read", URL: redactedURL, Err: err} } return b, nil } -func (p *proxyRepo) getBody(ctx context.Context, path string) (r io.ReadCloser, err error) { +func (p *proxyRepo) getBody(ctx context.Context, path string) (r io.ReadCloser, redactedURL string, err error) { fullPath := pathpkg.Join(p.url.Path, path) target := *p.url @@ -277,13 +281,13 @@ func (p *proxyRepo) getBody(ctx context.Context, path string) (r io.ReadCloser, resp, err := web.Get(web.DefaultSecurity, &target) if err != nil { - return nil, err + return nil, "", err } if err := resp.Err(); err != nil { resp.Body.Close() - return nil, err + return nil, "", err } - return resp.Body, nil + return resp.Body, resp.URL, nil } func (p *proxyRepo) Versions(ctx context.Context, prefix string) (*Versions, error) { @@ -370,7 +374,7 @@ func (p *proxyRepo) Stat(ctx context.Context, rev string) (*RevInfo, error) { } info := new(RevInfo) if err := json.Unmarshal(data, info); err != nil { - return nil, p.versionError(rev, fmt.Errorf("invalid response from proxy %q: %w", p.redactedURL, err)) + return nil, p.versionError(rev, fmt.Errorf("invalid response from proxy %q: %w", p.redactedBase, err)) } if info.Version != rev && rev == module.CanonicalVersion(rev) && module.Check(p.path, rev) == nil { // If we request a correct, appropriate version for the module path, the @@ -391,7 +395,7 @@ func (p *proxyRepo) Latest(ctx context.Context) (*RevInfo, error) { } info := new(RevInfo) if err := json.Unmarshal(data, info); err != nil { - return nil, p.versionError("", fmt.Errorf("invalid response from proxy %q: %w", p.redactedURL, err)) + return nil, p.versionError("", fmt.Errorf("invalid response from proxy %q: %w", p.redactedBase, err)) } return info, nil } @@ -422,7 +426,7 @@ func (p *proxyRepo) Zip(ctx context.Context, dst io.Writer, version string) erro return p.versionError(version, err) } path := "@v/" + encVer + ".zip" - body, err := p.getBody(ctx, path) + body, redactedURL, err := p.getBody(ctx, path) if err != nil { return p.versionError(version, err) } @@ -430,9 +434,9 @@ func (p *proxyRepo) Zip(ctx context.Context, dst io.Writer, version string) erro lr := &io.LimitedReader{R: body, N: codehost.MaxZipFile + 1} if _, err := io.Copy(dst, lr); err != nil { - // net/http doesn't add context to Body errors, so add it here. + // net/http doesn't add context to Body read errors, so add it here. // (See https://go.dev/issue/52727.) - err = &url.Error{Op: "read", URL: strings.TrimSuffix(p.redactedURL, "/") + "/" + path, Err: err} + err = &url.Error{Op: "read", URL: redactedURL, Err: err} return p.versionError(version, err) } if lr.N <= 0 {