Skip to content

feat(github): surface structured error identifiers on 4xx responses #512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
piekstra merged 1 commit into from
Jul 16, 2026
Merged

feat(github): surface structured error identifiers on 4xx responses #512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions internal/gitprovider/github/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@piekstra-dev piekstra-dev Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multi-error join ("; ".join over resource.field.code triples) and the 200-char truncation are new, non-trivial logic paths but have no test coverage — the added test in http_test.go only exercises a single-error payload. Two risks go unverified: (1) the join behavior with >1 errors, and (2) detail[:maxDetail] truncates by byte index, which can split a multi-byte rune mid-sequence if resource/field/code ever contain non-ASCII text, producing invalid UTF-8 in the surfaced error string. Add a test case with multiple errors entries to lock in the ; -joined format, and either truncate on a rune boundary (e.g. via a small loop or utf8.RuneCountInString-aware trim) or document why byte truncation is acceptable given the enum-only inputs.

Reply inline to this comment.

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)
Expand Down
30 changes: 30 additions & 0 deletions internal/gitprovider/github/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package github

import "testing"

func TestHTTPStatusErrorSurfacesSafeDetail(t *testing.T) {

@piekstra-dev piekstra-dev Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The four no-leak scenarios (structured triple, non-JSON, message-only, metadata-free JSON) are asserted sequentially with t.Fatalf in one test function; a failure in an earlier case aborts the rest, hiding whether later canary cases also regressed. Splitting into t.Run subtests (or at least using t.Errorf instead of t.Fatalf) would let all four no-leak assertions report independently, which matters here since these are the canary tests the PR description says caught a prior leak.

Reply inline to this comment.

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("<html>some raw error echoing content</html>"))
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)
}
}
Loading