diff --git a/internal/services/releases_test.go b/internal/services/releases_test.go index 8765f22c..2b628366 100644 --- a/internal/services/releases_test.go +++ b/internal/services/releases_test.go @@ -119,7 +119,7 @@ func TestReleaseDeleteReportsCompletedStep(t *testing.T) { }{ "release only": {wantCalls: []string{"/releases/5"}}, "release and tag": {deleteTag: true, wantCalls: []string{"/releases/5", "/tags/v1%2Fbeta"}}, - "release fails": {deleteTag: true, failRelease: true, wantCalls: []string{"/releases/5"}, wantErr: "unavailable"}, + "release fails": {deleteTag: true, failRelease: true, wantCalls: []string{"/releases/5"}, wantErr: "not found"}, "tag fails": {deleteTag: true, failTag: true, wantCalls: []string{"/releases/5", "/tags/v1%2Fbeta"}, wantErr: "release deleted, but tag deletion failed"}, } { t.Run(name, func(t *testing.T) { diff --git a/internal/services/sdk_test.go b/internal/services/sdk_test.go index 69c375ea..2fd7d971 100644 --- a/internal/services/sdk_test.go +++ b/internal/services/sdk_test.go @@ -5,6 +5,7 @@ package services import ( "context" + "errors" "net" "net/http" "net/http/httptest" @@ -32,6 +33,14 @@ func TestSDKCallsAuthenticateWithoutVersionProbe(t *testing.T) { require.True(t, renamed) } +// requireAPIError checks the request and server message an *transport.APIError reports. +func requireAPIError(t *testing.T, want transport.APIError, err error) { + t.Helper() + got, ok := errors.AsType[*transport.APIError](err) + require.True(t, ok, "%v is not an API error", err) + require.Equal(t, want, transport.APIError{Status: got.Status, Method: got.Method, Endpoint: got.Endpoint, Message: got.Message}) +} + func TestSDKErrorsMapToAPIErrors(t *testing.T) { for _, tc := range []struct { name, body, message string @@ -50,7 +59,7 @@ func TestSDKErrorsMapToAPIErrors(t *testing.T) { err := do(context.Background(), client, func(ctx context.Context, c *gitea.Client) (*gitea.Response, error) { return c.Repositories.DeleteRepo(ctx, "alice", "project") }) - require.Equal(t, &transport.APIError{Status: tc.status, Method: http.MethodDelete, Endpoint: "/api/v1/repos/alice/project", Message: tc.message}, err) + requireAPIError(t, transport.APIError{Status: tc.status, Method: http.MethodDelete, Endpoint: "/api/v1/repos/alice/project", Message: tc.message}, err) require.NotContains(t, err.Error(), "secret") }) } @@ -65,7 +74,7 @@ func TestSDKStatusOnlyFailureKeepsServerMessage(t *testing.T) { return c.Repositories.RenameRepoBranch(ctx, "alice", "project", "main", gitea.RenameRepoBranchOption{Name: "trunk"}) }) require.False(t, renamed) - require.Equal(t, &transport.APIError{Status: http.StatusUnprocessableEntity, Method: http.MethodPatch, Endpoint: "/api/v1/repos/alice/project/branches/main", Message: "branch trunk already exists"}, err) + requireAPIError(t, transport.APIError{Status: http.StatusUnprocessableEntity, Method: http.MethodPatch, Endpoint: "/api/v1/repos/alice/project/branches/main", Message: "branch trunk already exists"}, err) require.EqualError(t, err, "Gitea API returned HTTP 422 for PATCH /api/v1/repos/alice/project/branches/main: branch trunk already exists") } @@ -78,7 +87,7 @@ func TestSDKFailureWithoutResponseKeepsCapturedError(t *testing.T) { _, _, err := c.Repositories.GetRepo(ctx, "alice", "project") return nil, nil, err }) - require.Equal(t, &transport.APIError{Status: http.StatusNotFound, Method: http.MethodGet, Endpoint: "/api/v1/repos/alice/project", Message: "repository does not exist"}, err) + requireAPIError(t, transport.APIError{Status: http.StatusNotFound, Method: http.MethodGet, Endpoint: "/api/v1/repos/alice/project", Message: "repository does not exist"}, err) } func TestSDKInvalidJSONIsReportedWithoutBody(t *testing.T) { diff --git a/internal/transport/api.go b/internal/transport/api.go index e49f7ed6..248dd199 100644 --- a/internal/transport/api.go +++ b/internal/transport/api.go @@ -54,7 +54,9 @@ func (t captureTransport) RoundTrip(req *http.Request) (*http.Response, error) { return resp, err } data, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBody)) - holder.Store(newAPIError(resp.StatusCode, req.Method, req.URL.Path, t.token, data)) + failure := newAPIError(resp.StatusCode, req.Method, req.URL.Path, t.token, data) + failure.bind(req.URL, t.base) + holder.Store(failure) resp.Body = struct { io.Reader io.Closer diff --git a/internal/transport/api_test.go b/internal/transport/api_test.go index edfad204..f04c26ab 100644 --- a/internal/transport/api_test.go +++ b/internal/transport/api_test.go @@ -172,7 +172,12 @@ func TestCaptureErrorsSanitizesAndKeepsBodyReadable(t *testing.T) { require.NoError(t, err) require.NoError(t, resp.Body.Close()) require.Equal(t, tc.body, string(data)) - require.Equal(t, &APIError{Status: tc.status, Method: http.MethodPatch, Endpoint: "/api/v1/repos/a/b", Message: tc.message}, failure()) + recorded := failure() + require.Equal(t, tc.status, recorded.Status) + require.Equal(t, http.MethodPatch, recorded.Method) + require.Equal(t, "/api/v1/repos/a/b", recorded.Endpoint) + require.Equal(t, tc.message, recorded.Message) + require.Equal(t, server.URL, recorded.Instance) }) } } diff --git a/internal/transport/client.go b/internal/transport/client.go index b8441097..234f453d 100644 --- a/internal/transport/client.go +++ b/internal/transport/client.go @@ -32,6 +32,12 @@ type RequestSigner interface { type APIError struct { Status int Method, Endpoint, Message string + // Instance is the installation URL the failed request was sent to, when it was recorded + // from a request. + Instance string + // apiBase and lookup repeat reads on the failed request's instance with its credential. + apiBase string + lookup http.RoundTripper } func (e *APIError) Error() string { @@ -42,7 +48,11 @@ func (e *APIError) Error() string { case 403: text = "permission denied by the Gitea instance" case 404: - text = "resource or API endpoint unavailable on the selected Gitea instance" + // Gitea's 404 message is usually just "not found"; the request names what was missing. + text = "not found (" + e.request() + ")" + if strings.EqualFold(e.Message, "not found") { + return text + } case 429: text = "Gitea rate limit exceeded; try again later" default: @@ -54,9 +64,52 @@ func (e *APIError) Error() string { return text } +// request describes the failed request, such as "HTTP 404 GET /api/v1/repos/alice/x". +func (e *APIError) request() string { + return strings.TrimSpace(fmt.Sprintf("HTTP %d %s %s", e.Status, e.Method, e.Endpoint)) +} + +// bind records the instance of the request to u and the credentialed transport rt, so the +// failure can later be explained with Lookup. +func (e *APIError) bind(u *url.URL, rt http.RoundTripper) { + prefix, _, ok := strings.Cut(u.Path, "/api/v1/") + if !ok { + return + } + instance := url.URL{Scheme: u.Scheme, Host: u.Host, Path: prefix} + e.Instance = instance.String() + instance.Path += "/api/v1/" + e.apiBase, e.lookup = instance.String(), rt +} + +// Lookup reads endpoint, a path below the API base such as "repos/alice/project", with the +// instance and credential of the request that failed, and decodes a successful JSON response +// into out. It returns the response status, or 0 when the error was not recorded from a +// request or the lookup itself failed. +func (e *APIError) Lookup(ctx context.Context, endpoint string, out any) int { + if e.lookup == nil { + return 0 + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, e.apiBase+strings.TrimLeft(endpoint, "/"), nil) + if err != nil { + return 0 + } + req.Header.Set("Accept", "application/json") + resp, err := e.lookup.RoundTrip(req) + if err != nil { + return 0 + } + defer resp.Body.Close() + if resp.StatusCode >= 200 && resp.StatusCode < 300 && out != nil && json.NewDecoder(io.LimitReader(resp.Body, maxLookupBody)).Decode(out) != nil { + return 0 + } + return resp.StatusCode +} + const ( maxErrorBody = 4 << 10 maxErrorMessage = 300 + maxLookupBody = 1 << 20 ) // newAPIError keeps only a bounded, printable JSON "message" field of a failed response body. @@ -175,6 +228,20 @@ func (c *Client) URL(endpoint string) (string, error) { return base.String(), nil } +// RequestFailure reports a request that received no HTTP response, such as a refused +// connection or an untrusted certificate. +type RequestFailure struct { + // Action names what failed, such as "API request failed". + Action string + // Server is the scheme and host the request was sent to, without path or query. + Server string + // Err is the network or TLS cause. + Err error +} + +func (e *RequestFailure) Error() string { return e.Action + ": " + e.Err.Error() } +func (e *RequestFailure) Unwrap() error { return e.Err } + // RequestError keeps the network or TLS cause of a failed request. The cause of a *url.Error // omits the request URL, whose query may carry secrets. func RequestError(ctx context.Context, action string, err error) error { @@ -182,7 +249,11 @@ func RequestError(ctx context.Context, action string, err error) error { return ctx.Err() } if urlErr, ok := errors.AsType[*url.Error](err); ok { - return fmt.Errorf("%s: %w", action, urlErr.Err) + failure := &RequestFailure{Action: action, Err: urlErr.Err} + if u, parseErr := url.Parse(urlErr.URL); parseErr == nil { + failure.Server = (&url.URL{Scheme: u.Scheme, Host: u.Host}).String() + } + return failure } return errors.New(action + "; check connectivity and TLS settings") } diff --git a/internal/transport/client_test.go b/internal/transport/client_test.go index 357d147d..3363f4c5 100644 --- a/internal/transport/client_test.go +++ b/internal/transport/client_test.go @@ -71,6 +71,8 @@ func TestAPIErrorIncludesOnlySanitizedServerMessage(t *testing.T) { {"oversized body", `{"message":"` + strings.Repeat("y", 5000) + `"}`, fmt.Sprintf(merge, 500), 500}, {"token echo", `{"message":"bad token secret"}`, fmt.Sprintf(merge, 400), 400}, {"authentication", `{"message":"user alice not found"}`, "authentication rejected; run tea auth login", 401}, + {"bare not found", `{"message":"not found","url":"https://leak.example/?key=abc"}`, "not found (HTTP 404 POST /api/v1/repos/a/b/pulls/1/merge)", 404}, + {"described not found", `{"message":"label does not exist [label_id: 9]"}`, "not found (HTTP 404 POST /api/v1/repos/a/b/pulls/1/merge): label does not exist [label_id: 9]", 404}, } { t.Run(tc.name, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -146,6 +148,9 @@ func TestNetworkErrorsKeepCauseWithoutURL(t *testing.T) { require.ErrorAs(t, err, &opErr) require.ErrorContains(t, err, "API request failed: ") require.NotContains(t, err.Error(), "query-secret") + var failure *RequestFailure + require.ErrorAs(t, err, &failure) + require.Equal(t, server.URL, failure.Server) _, err = client.Download(context.Background(), server.URL+"/file?signature=query-secret") require.ErrorAs(t, err, &opErr) require.ErrorContains(t, err, "download failed: ") @@ -153,14 +158,86 @@ func TestNetworkErrorsKeepCauseWithoutURL(t *testing.T) { } func TestDebugLogsRequestsWithoutSecrets(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTeapot) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/repos/a/b/labels/8": + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"label does not exist [label_id: 8]"}`)) + case "/api/v1/repos/a/b/labels/9": + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"label does not exist; secret-token"}`)) + default: + w.WriteHeader(http.StatusTeapot) + } })) defer server.Close() var log bytes.Buffer client := New(server.URL, "secret-token", false).Debug(&log) - _, err := client.Do(context.Background(), http.MethodGet, "repos/a/b?page=2&token=query-secret", nil, nil) + _, err := client.Do(context.Background(), http.MethodGet, "repos/a/b?page=2&state=open&token=query-secret&access_token=query-secret", nil, nil) require.Error(t, err) - require.Regexp(t, `^\[debug\] GET `+regexp.QuoteMeta(server.URL)+`/api/v1/repos/a/b\?page=REDACTED&token=REDACTED -> 418 I'm a teapot \(\d+(\.\d+)?[µm]?s\)\n$`, log.String()) + require.Regexp(t, `^\[debug\] GET `+regexp.QuoteMeta(server.URL)+`/api/v1/repos/a/b\?access_token=REDACTED&page=2&state=open&token=REDACTED -> 418 I'm a teapot \(\d+(\.\d+)?[µm]?s\)\n$`, log.String()) + log.Reset() + _, err = client.Do(context.Background(), http.MethodGet, "repos/a/b/labels/8", nil, nil) + require.Regexp(t, `-> 404 Not Found \(\d+(\.\d+)?[µm]?s\): label does not exist \[label_id: 8\]\n$`, log.String()) + var apiErr *APIError + require.ErrorAs(t, err, &apiErr) + require.Equal(t, "label does not exist [label_id: 8]", apiErr.Message, "the logged body stays readable") + log.Reset() + _, err = client.Do(context.Background(), http.MethodGet, "repos/a/b/labels/9", nil, nil) + require.Error(t, err) + require.Regexp(t, `-> 404 Not Found \(\d+(\.\d+)?[µm]?s\)\n$`, log.String(), "a message echoing the token is dropped") require.NotContains(t, log.String(), "secret") } + +func TestDebugRedactsOnlySensitiveQueryValues(t *testing.T) { + for _, tc := range []struct { + path, key string + redacted bool + }{ + {"/api/v1/repos/a/b/issues", "limit", false}, + {"/api/v1/repos/a/b/issues", "page", false}, + {"/api/v1/repos/a/b/issues", "state", false}, + {"/api/v1/repos/a/b/issues", "labels", false}, + {"/api/v1/repos/a/b/issues", "token", true}, + {"/api/v1/repos/a/b/issues", "access_token", true}, + {"/api/v1/users", "password", true}, + {"/login/oauth/authorize", "state", true}, + {"/login/oauth/access_token", "code", true}, + {"/login/oauth/access_token", "client_secret", true}, + {"/storage/blob", "X-Amz-Signature", true}, + {"/storage/blob", "X-Amz-Credential", true}, + {"/storage/blob", "X-Amz-Security-Token", true}, + {"/storage/blob", "sig", true}, + {"/storage/blob", "X-Amz-Date", false}, + } { + require.Equal(t, tc.redacted, sensitiveQuery(tc.path, tc.key), "%s %s", tc.path, tc.key) + } +} + +func TestAPIErrorLookupRepeatsReadsWithTheFailedCredential(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "token secret", r.Header.Get("Authorization")) + require.Equal(t, "bob", r.Header.Get("Sudo")) + if r.URL.Path == "/base/api/v1/repos/a/b" { + _, _ = w.Write([]byte(`{"full_name":"a/b","has_issues":false}`)) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"not found"}`)) + })) + defer server.Close() + client := New(server.URL+"/base", "secret", false) + client.Sudo = "bob" + _, err := client.Do(context.Background(), http.MethodGet, "repos/a/b/issues/9", nil, nil) + var apiErr *APIError + require.ErrorAs(t, err, &apiErr) + require.Equal(t, server.URL+"/base", apiErr.Instance) + var repo struct { + FullName string `json:"full_name"` + HasIssues bool `json:"has_issues"` + } + require.Equal(t, http.StatusOK, apiErr.Lookup(context.Background(), "repos/a/b", &repo)) + require.Equal(t, "a/b", repo.FullName) + require.Equal(t, http.StatusNotFound, apiErr.Lookup(context.Background(), "repos/a/missing", nil)) + require.Zero(t, (&APIError{Status: http.StatusNotFound}).Lookup(context.Background(), "repos/a/b", nil), "errors built without a request cannot look up") +} diff --git a/internal/transport/debug.go b/internal/transport/debug.go index 34e913d5..141594c8 100644 --- a/internal/transport/debug.go +++ b/internal/transport/debug.go @@ -4,10 +4,12 @@ package transport import ( + "bytes" "fmt" "io" "net/http" "net/url" + "strings" "time" ) @@ -19,32 +21,74 @@ type debugTransport struct { func (t debugTransport) RoundTrip(req *http.Request) (*http.Response, error) { start := time.Now() resp, err := t.base.RoundTrip(req) - var outcome string + var outcome, message string if err != nil { outcome = "error: " + err.Error() } else { outcome = resp.Status + message = failureMessage(req, resp) } - _, _ = fmt.Fprintf(t.out, "[debug] %s %s -> %s (%s)\n", req.Method, redactURL(req.URL), outcome, time.Since(start).Round(time.Millisecond)) + line := fmt.Sprintf("[debug] %s %s -> %s (%s)", req.Method, redactURL(req.URL), outcome, time.Since(start).Round(time.Millisecond)) + if message != "" { + line += ": " + message + } + _, _ = fmt.Fprintln(t.out, line) return resp, err } -// redactURL drops userinfo and query values, which can carry tokens or signatures. +// failureMessage returns the sanitized server message of a failed response and keeps its body +// readable, so --debug shows what the server said when tea explains the failure in its own words. +func failureMessage(req *http.Request, resp *http.Response) string { + if resp.StatusCode < 400 { + return "" + } + data, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBody)) + resp.Body = struct { + io.Reader + io.Closer + }{io.MultiReader(bytes.NewReader(data), resp.Body), resp.Body} + token := strings.TrimPrefix(req.Header.Get("Authorization"), "token ") + return newAPIError(resp.StatusCode, req.Method, req.URL.Path, token, data).Message +} + +// redactURL drops userinfo and the values of query parameters that can carry credentials or +// signatures, keeping filters such as limit, page and state readable. func redactURL(u *url.URL) string { clean := *u clean.User = nil if clean.RawQuery != "" { query := clean.Query() for key := range query { - query[key] = []string{"REDACTED"} + if sensitiveQuery(clean.Path, key) { + query[key] = []string{"REDACTED"} + } } clean.RawQuery = query.Encode() } return clean.String() } +// sensitiveQuery reports query parameters whose values can authenticate a request: tokens, +// passwords, secrets, signatures and credentials of signed storage URLs, and OAuth codes and state. +func sensitiveQuery(path, key string) bool { + key = strings.ToLower(key) + switch { + case key == "code" || key == "key" || strings.HasPrefix(key, "sig"): + return true + case key == "state": + return strings.Contains(path, "/login/oauth/") + } + for _, word := range []string{"token", "password", "secret", "signature", "credential"} { + if strings.Contains(key, word) { + return true + } + } + return false +} + // Debug returns a copy of c that logs the method, redacted URL, status, and duration of each -// request to w. Headers and bodies are never logged. +// request to w, plus the sanitized server message of a failed response. Headers and bodies are +// never logged. func (c *Client) Debug(w io.Writer) *Client { clone := *c httpClient := *c.HTTP