From 518e78db54058b15e4909538a1b31b92a148a0a8 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 15:56:32 -0400 Subject: [PATCH] Require secrets for ProviderError redaction and omit unredactable bodies NewProviderError now takes the secrets to redact as a required []string instead of an optional variadic. When no non-empty secret is given the body cannot be redacted, so no excerpt is stored and Body()/Error() never expose raw vendor output. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/errors.go | 20 ++++++++++++++------ internal/ai/llm/errors_test.go | 27 ++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index 723eaf2..be8e922 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -56,17 +56,24 @@ type ProviderError struct { body string } -// NewProviderError returns the error for an unexpected HTTP status. Every -// occurrence of each non-empty secret (the API key, at least) in body is -// replaced before the body is cut to MaxBodyExcerpt bytes, so no part of a -// secret survives. -func NewProviderError(status int, body []byte, secrets ...string) *ProviderError { +// NewProviderError returns the error for an unexpected HTTP status. +// secrets must hold the API key (and any other credential sent with the +// request). Every occurrence of each non-empty secret in body is replaced +// before the body is cut to MaxBodyExcerpt bytes, so no part of a secret +// survives. If secrets holds no non-empty value the body cannot be +// redacted, so no excerpt is kept at all. +func NewProviderError(status int, body []byte, secrets []string) *ProviderError { s := string(body) + redactedAny := false for _, secret := range secrets { if secret != "" { s = strings.ReplaceAll(s, secret, redacted) + redactedAny = true } } + if !redactedAny { + return &ProviderError{status: status} + } s = strings.TrimSpace(s) if len(s) > MaxBodyExcerpt { cut := MaxBodyExcerpt @@ -81,7 +88,8 @@ func NewProviderError(status int, body []byte, secrets ...string) *ProviderError // Status returns the HTTP status code. func (e *ProviderError) Status() int { return e.status } -// Body returns the redacted, truncated response-body excerpt. +// Body returns the redacted, truncated response-body excerpt; it is empty +// when the vendor sent no body or no secret was given to redact it with. func (e *ProviderError) Body() string { return e.body } func (e *ProviderError) Error() string { diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index ba8adc9..321f887 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -46,7 +46,7 @@ func TestRateLimitedError(t *testing.T) { } func TestProviderError(t *testing.T) { - err := fmt.Errorf("gemini: %w", NewProviderError(503, []byte(` {"error":"overloaded"} `))) + err := fmt.Errorf("gemini: %w", NewProviderError(503, []byte(` {"error":"overloaded"} `), []string{"sk-test"})) if !errors.Is(err, ErrProvider) { t.Fatal("errors.Is(err, ErrProvider) = false") } @@ -63,7 +63,7 @@ func TestProviderError(t *testing.T) { if got := err.Error(); got != `gemini: llm: provider error: HTTP 503: {"error":"overloaded"}` { t.Fatalf("Error() = %q", got) } - if got := NewProviderError(500, nil).Error(); got != "llm: provider error: HTTP 500" { + if got := NewProviderError(500, nil, []string{"sk-test"}).Error(); got != "llm: provider error: HTTP 500" { t.Fatalf("Error() without body = %q", got) } } @@ -71,7 +71,7 @@ func TestProviderError(t *testing.T) { func TestProviderErrorRedactsSecrets(t *testing.T) { const key = "sk-live-0123456789abcdef" body := []byte(`{"error":"invalid key sk-live-0123456789abcdef","echo":"Bearer sk-live-0123456789abcdef"}`) - pe := NewProviderError(400, body, key, "") + pe := NewProviderError(400, body, []string{key, ""}) for _, s := range []string{pe.Error(), pe.Body()} { if strings.Contains(s, key) { t.Fatalf("secret leaked: %q", s) @@ -88,7 +88,7 @@ func TestProviderErrorRedactsBeforeTruncating(t *testing.T) { // The key starts just before the excerpt limit, so truncating first // would leave its prefix in the excerpt. body := strings.Repeat("x", MaxBodyExcerpt-5) + key - pe := NewProviderError(500, []byte(body), key) + pe := NewProviderError(500, []byte(body), []string{key}) if strings.Contains(pe.Body(), "sk-se") { t.Fatalf("secret prefix leaked: %q", pe.Body()) } @@ -96,9 +96,26 @@ func TestProviderErrorRedactsBeforeTruncating(t *testing.T) { func TestProviderErrorTruncatesOnRuneBoundary(t *testing.T) { body := strings.Repeat("a", MaxBodyExcerpt-1) + "é" + strings.Repeat("b", 100) - pe := NewProviderError(500, []byte(body)) + pe := NewProviderError(500, []byte(body), []string{"sk-test"}) want := strings.Repeat("a", MaxBodyExcerpt-1) + "…" if pe.Body() != want { t.Fatalf("Body() has %d bytes, want %d", len(pe.Body()), len(want)) } } + +func TestProviderErrorOmitsBodyWithoutSecret(t *testing.T) { + const key = "sk-live-0123456789abcdef" + body := []byte(`{"error":"invalid key sk-live-0123456789abcdef"}`) + for _, secrets := range [][]string{nil, {}, {""}} { + pe := NewProviderError(401, body, secrets) + if pe.Body() != "" { + t.Fatalf("secrets %q: Body() = %q, want empty", secrets, pe.Body()) + } + if got := pe.Error(); got != "llm: provider error: HTTP 401" { + t.Fatalf("secrets %q: Error() = %q", secrets, got) + } + if strings.Contains(pe.Error(), key) { + t.Fatalf("secrets %q: secret leaked: %q", secrets, pe.Error()) + } + } +}