diff --git a/internal/gitprovider/github/http.go b/internal/gitprovider/github/http.go index e9bc4427..878be122 100644 --- a/internal/gitprovider/github/http.go +++ b/internal/gitprovider/github/http.go @@ -236,9 +236,49 @@ func httpStatusError(status int, body []byte) error { if len(body) == 0 { return fmt.Errorf("github: status %d", status) } + if detail := safeErrorDetail(body); detail != "" { + return fmt.Errorf("github: status %d (%s)", status, detail) + } return fmt.Errorf("github: status %d (response body redacted)", status) } +// safeErrorDetail extracts only GitHub's structured error identifiers: each +// error's resource/field/code triple. These are schema enums, never free text. +// The top-level message and raw body are deliberately excluded — both can echo +// request content or secret material (the no-leak canary tests enforce this). +// Without the triples a 4xx is undiagnosable ("response body redacted") even +// though GitHub named exactly which field it rejected. +func safeErrorDetail(body []byte) string { + var payload struct { + Errors []struct { + Resource string `json:"resource"` + Field string `json:"field"` + Code string `json:"code"` + } `json:"errors"` + } + if err := json.Unmarshal(body, &payload); err != nil { + return "" + } + parts := make([]string, 0, len(payload.Errors)) + for _, entry := range payload.Errors { + fields := make([]string, 0, 3) + for _, value := range []string{entry.Resource, entry.Field, entry.Code} { + if value = strings.TrimSpace(value); value != "" { + fields = append(fields, value) + } + } + if len(fields) > 0 { + parts = append(parts, strings.Join(fields, ".")) + } + } + const maxDetail = 200 + detail := strings.Join(parts, "; ") + if len(detail) > maxDetail { + detail = detail[:maxDetail] + "…" + } + return detail +} + func restURL(base *url.URL, parts ...string) string { copied := *base escaped := make([]string, 0, len(parts)+1) diff --git a/internal/gitprovider/github/http_test.go b/internal/gitprovider/github/http_test.go new file mode 100644 index 00000000..24a68fce --- /dev/null +++ b/internal/gitprovider/github/http_test.go @@ -0,0 +1,30 @@ +package github + +import "testing" + +func TestHTTPStatusErrorSurfacesSafeDetail(t *testing.T) { + body := []byte(`{"message":"Validation Failed","errors":[{"resource":"PullRequestReview","field":"path","code":"invalid"}],"documentation_url":"https://docs.github.com"}`) + err := httpStatusError(400, body) + want := "github: status 400 (PullRequestReview.path.invalid)" + if err == nil || err.Error() != want { + t.Fatalf("err = %v, want %q", err, want) + } + + // Non-JSON bodies stay fully redacted — they can echo request content. + err = httpStatusError(400, []byte("some raw error echoing content")) + if err == nil || err.Error() != "github: status 400 (response body redacted)" { + t.Fatalf("non-JSON err = %v, want redacted", err) + } + + // Free-text message alone is NOT surfaced — it can echo secrets/content. + err = httpStatusError(401, []byte(`{"message":"bad credentials ghp_canary"}`)) + if err == nil || err.Error() != "github: status 401 (response body redacted)" { + t.Fatalf("message-only err = %v, want redacted", err) + } + + // JSON without errors metadata also stays redacted. + err = httpStatusError(422, []byte(`{"data":"user content"}`)) + if err == nil || err.Error() != "github: status 422 (response body redacted)" { + t.Fatalf("metadata-free err = %v, want redacted", err) + } +}