From ec1a591b712d0690a2744404522478c45ae14483 Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 17 Mar 2026 16:24:37 +0000 Subject: [PATCH] fix(coderabbit): address review findings - Truncate response body in error messages to 256 bytes to prevent sensitive data leakage from upstream APIs (client.go) - Replace unchecked strings.Split index with safe repoShortName helper to prevent potential panic (cmd/monitor) - Fix test assertions to match coreerr.E error format after prior refactor Co-Authored-By: Virgil --- client.go | 19 +++++++++++++++---- client_test.go | 6 +++--- cloudns_test.go | 4 ++-- cmd/monitor/cmd_monitor.go | 17 +++++++++++++---- hetzner_test.go | 6 +++--- 5 files changed, 36 insertions(+), 16 deletions(-) diff --git a/client.go b/client.go index d5f29c8..1a7af1c 100644 --- a/client.go +++ b/client.go @@ -144,7 +144,7 @@ func (a *APIClient) Do(req *http.Request, result any) error { // Server errors are retryable. if resp.StatusCode >= 500 { - lastErr = coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, string(data)), nil) + lastErr = coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, truncateBody(data, maxErrBodyLen)), nil) if attempt < attempts-1 { a.backoff(attempt, req) } @@ -153,7 +153,7 @@ func (a *APIClient) Do(req *http.Request, result any) error { // Client errors (4xx, except 429 handled above) are not retried. if resp.StatusCode >= 400 { - return coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, string(data)), nil) + return coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, truncateBody(data, maxErrBodyLen)), nil) } // Success — decode if requested. @@ -228,7 +228,7 @@ func (a *APIClient) DoRaw(req *http.Request) ([]byte, error) { } if resp.StatusCode >= 500 { - lastErr = coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, string(data)), nil) + lastErr = coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, truncateBody(data, maxErrBodyLen)), nil) if attempt < attempts-1 { a.backoff(attempt, req) } @@ -236,7 +236,7 @@ func (a *APIClient) DoRaw(req *http.Request) ([]byte, error) { } if resp.StatusCode >= 400 { - return nil, coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, string(data)), nil) + return nil, coreerr.E(a.prefix, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, truncateBody(data, maxErrBodyLen)), nil) } return data, nil @@ -261,6 +261,17 @@ func (a *APIClient) backoff(attempt int, req *http.Request) { } } +// maxErrBodyLen is the maximum number of bytes from a response body included in error messages. +const maxErrBodyLen = 256 + +// truncateBody limits response body length in error messages to prevent sensitive data leakage. +func truncateBody(data []byte, max int) string { + if len(data) <= max { + return string(data) + } + return string(data[:max]) + "...(truncated)" +} + // parseRetryAfter interprets the Retry-After header value. // Supports seconds (integer) format. Falls back to 1 second. func parseRetryAfter(val string) time.Duration { diff --git a/client_test.go b/client_test.go index 13db078..619af96 100644 --- a/client_test.go +++ b/client_test.go @@ -137,7 +137,7 @@ func TestAPIClient_Do_Bad_ClientError(t *testing.T) { err = c.Do(req, nil) assert.Error(t, err) - assert.Contains(t, err.Error(), "test-api 404") + assert.Contains(t, err.Error(), "test-api: HTTP 404") assert.Contains(t, err.Error(), "not found") } @@ -226,7 +226,7 @@ func TestAPIClient_Do_Bad_ExhaustsRetries(t *testing.T) { err = c.Do(req, nil) assert.Error(t, err) - assert.Contains(t, err.Error(), "exhaust-test 500") + assert.Contains(t, err.Error(), "exhaust-test: HTTP 500") // 1 initial + 2 retries = 3 attempts assert.Equal(t, int32(3), attempts.Load()) } @@ -476,7 +476,7 @@ func TestAPIClient_DoRaw_Bad_ClientError(t *testing.T) { _, err = c.DoRaw(req) assert.Error(t, err) - assert.Contains(t, err.Error(), "raw-test 403") + assert.Contains(t, err.Error(), "raw-test: HTTP 403") } func TestAPIClient_DoRaw_Good_RetriesServerError(t *testing.T) { diff --git a/cloudns_test.go b/cloudns_test.go index 3436f95..7fbf2b1 100644 --- a/cloudns_test.go +++ b/cloudns_test.go @@ -78,7 +78,7 @@ func TestCloudNSClient_DoRaw_Bad_HTTPError(t *testing.T) { _, err = client.doRaw(req) assert.Error(t, err) - assert.Contains(t, err.Error(), "cloudns API 403") + assert.Contains(t, err.Error(), "cloudns API: HTTP 403") } func TestCloudNSClient_DoRaw_Bad_ServerError(t *testing.T) { @@ -102,7 +102,7 @@ func TestCloudNSClient_DoRaw_Bad_ServerError(t *testing.T) { _, err = client.doRaw(req) assert.Error(t, err) - assert.Contains(t, err.Error(), "cloudns API 500") + assert.Contains(t, err.Error(), "cloudns API: HTTP 500") } // --- Zone JSON parsing --- diff --git a/cmd/monitor/cmd_monitor.go b/cmd/monitor/cmd_monitor.go index 7b144bf..482fb84 100644 --- a/cmd/monitor/cmd_monitor.go +++ b/cmd/monitor/cmd_monitor.go @@ -210,7 +210,7 @@ func resolveRepos() ([]string, error) { func fetchRepoFindings(repoFullName string) ([]Finding, []string) { var findings []Finding var errs []string - repoName := strings.Split(repoFullName, "/")[1] + repoName := repoShortName(repoFullName) // Fetch code scanning alerts codeFindings, err := fetchCodeScanningAlerts(repoFullName) @@ -264,7 +264,7 @@ func fetchCodeScanningAlerts(repoFullName string) ([]Finding, error) { return nil, log.E("monitor.fetchCodeScanning", "failed to parse response", err) } - repoName := strings.Split(repoFullName, "/")[1] + repoName := repoShortName(repoFullName) var findings []Finding for _, alert := range alerts { if alert.State != "open" { @@ -318,7 +318,7 @@ func fetchDependabotAlerts(repoFullName string) ([]Finding, error) { return nil, log.E("monitor.fetchDependabot", "failed to parse response", err) } - repoName := strings.Split(repoFullName, "/")[1] + repoName := repoShortName(repoFullName) var findings []Finding for _, alert := range alerts { if alert.State != "open" { @@ -369,7 +369,7 @@ func fetchSecretScanningAlerts(repoFullName string) ([]Finding, error) { return nil, log.E("monitor.fetchSecretScanning", "failed to parse response", err) } - repoName := strings.Split(repoFullName, "/")[1] + repoName := repoShortName(repoFullName) var findings []Finding for _, alert := range alerts { if alert.State != "open" { @@ -530,6 +530,15 @@ func outputTable(findings []Finding) error { return nil } +// repoShortName extracts the repo name from "org/repo" format. +// Returns the full string if no "/" is present. +func repoShortName(fullName string) string { + if i := strings.LastIndex(fullName, "/"); i >= 0 { + return fullName[i+1:] + } + return fullName +} + // truncate truncates a string to max runes (Unicode-safe) func truncate(s string, max int) string { runes := []rune(s) diff --git a/hetzner_test.go b/hetzner_test.go index 204c76e..42fb892 100644 --- a/hetzner_test.go +++ b/hetzner_test.go @@ -116,7 +116,7 @@ func TestHCloudClient_Do_Bad_APIError(t *testing.T) { var result struct{} err := client.get(context.Background(), "/servers", &result) assert.Error(t, err) - assert.Contains(t, err.Error(), "hcloud API 403") + assert.Contains(t, err.Error(), "hcloud API: HTTP 403") } func TestHCloudClient_Do_Bad_APIErrorNoJSON(t *testing.T) { @@ -136,7 +136,7 @@ func TestHCloudClient_Do_Bad_APIErrorNoJSON(t *testing.T) { err := client.get(context.Background(), "/servers", nil) assert.Error(t, err) - assert.Contains(t, err.Error(), "hcloud API 500") + assert.Contains(t, err.Error(), "hcloud API: HTTP 500") } func TestHCloudClient_Do_Good_NilResult(t *testing.T) { @@ -218,7 +218,7 @@ func TestHRobotClient_Get_Bad_HTTPError(t *testing.T) { err := client.get(context.Background(), "/server", nil) assert.Error(t, err) - assert.Contains(t, err.Error(), "hrobot API 401") + assert.Contains(t, err.Error(), "hrobot API: HTTP 401") } // --- Type serialisation --- -- 2.45.3