From d11cdbbefb655939696ad674e21dd7060f456646 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 08:05:17 -0400 Subject: [PATCH] Keep %#v of every LLM adapter error free of the API key (MQ-03) %#v prints struct field names, so a key that spells one (for example "body" on ProviderError) leaked even though Error() was redacted. - llm.ProviderError and llm.RateLimitedError gain GoString returning their already-redacted Error text. - Both adapters' transportFailure now holds its fully redacted text and gains GoString. - The redact backstop now wraps every non-nil error when a key is set, so the returned error and every unwrapped layer print through redactedError.GoString, never a raw struct such as fmt.wrapError. - redactedError.As also refuses a match whose %#v form holds the key. - Contract test in both adapters: field-name keys (body, status, text, RetryAfter, msg, err, key, kind) on HTTP 500 "down" and 429, checking %#v of the returned error, every unwrapped layer and errors.As results. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/anthropic/anthropic.go | 38 +++++++------ internal/ai/llm/anthropic/anthropic_test.go | 59 +++++++++++++++++++-- internal/ai/llm/errors.go | 7 +++ internal/ai/llm/openai/openai.go | 38 +++++++------ internal/ai/llm/openai/openai_test.go | 59 +++++++++++++++++++-- 5 files changed, 160 insertions(+), 41 deletions(-) diff --git a/internal/ai/llm/anthropic/anthropic.go b/internal/ai/llm/anthropic/anthropic.go index 3625db8..37b1239 100644 --- a/internal/ai/llm/anthropic/anthropic.go +++ b/internal/ai/llm/anthropic/anthropic.go @@ -437,16 +437,14 @@ func isTimeout(err error) bool { // with the request URL removed and the API key redacted, and unwraps only to // the cause's category: llm.ErrTimeout, context.Canceled or nothing. type transportFailure struct { - detail string - kind error + text string + kind error } -func (e *transportFailure) Error() string { - if e.kind == llm.ErrTimeout { - return fmt.Sprintf("%s: transport: %s", llm.ErrTimeout, e.detail) - } - return "transport: " + e.detail -} +func (e *transportFailure) Error() string { return e.text } + +// GoString keeps %#v to the redacted Error text instead of the fields. +func (e *transportFailure) GoString() string { return e.text } func (e *transportFailure) Unwrap() error { return e.kind } @@ -465,7 +463,11 @@ func (p *Provider) transportError(err error) error { if errors.As(err, &ue) { err = ue.Err } - return &transportFailure{detail: llm.Redact(err.Error(), p.apiKey), kind: kind} + text := "transport: " + err.Error() + if kind == llm.ErrTimeout { + text = fmt.Sprintf("%s: %s", llm.ErrTimeout, text) + } + return &transportFailure{text: llm.Redact(text, p.apiKey), kind: kind} } // redactedError stands in for one layer of an error chain that holds the @@ -474,8 +476,8 @@ func (p *Provider) transportError(err error) error { // unwrapping has text holding the key. Is consults the original layer and // its chain, so sentinels still match. As does too. The llm typed errors // remove the key from their own complete text, fixed words included, so -// they always match; As refuses only a match from outside llm whose text -// holds the key, so it never hands back an unredacted error. +// they always match; As refuses a match whose text, or whose %#v form, +// holds the key, so it never hands back an error that can print it. type redactedError struct { text string err error @@ -496,7 +498,8 @@ func (e *redactedError) As(target any) bool { return false } found := reflect.ValueOf(target).Elem() - if matched, ok := found.Interface().(error); ok && holdsKey(matched, e.key) { + matched, isErr := found.Interface().(error) + if (isErr && holdsKey(matched, e.key)) || strings.Contains(fmt.Sprintf("%#v", found.Interface()), e.key) { found.SetZero() return false } @@ -518,12 +521,13 @@ func (e *redactedJoin) Unwrap() []error { return safe } -// redact is the backstop every exported method applies to its error: when -// the text of err or of any error in its chain holds a non-empty key, it -// returns a stand-in whose text, and the text of every error it unwraps -// to, has the key redacted. Any other err is returned unchanged. +// redact is the backstop every exported method applies to its error: with +// a non-empty key, it returns a stand-in whose text, and the text of every +// error it unwraps to, has the key redacted. Every non-nil err is wrapped, +// even one whose text lacks the key, because %#v of an unwrapped error +// prints its struct field names, and a key may spell one. func redact(err error, key string) error { - if err == nil || key == "" || !holdsKey(err, key) { + if err == nil || key == "" { return err } re := redactedError{text: llm.Redact(err.Error(), key), err: err, key: key} diff --git a/internal/ai/llm/anthropic/anthropic_test.go b/internal/ai/llm/anthropic/anthropic_test.go index ccae5b1..e2bab59 100644 --- a/internal/ai/llm/anthropic/anthropic_test.go +++ b/internal/ai/llm/anthropic/anthropic_test.go @@ -872,7 +872,7 @@ func TestRedactKeepsChainMatches(t *testing.T) { pe := llm.NewProviderError(500, []byte(`{"error":"down"}`), testKey) rl := llm.NewRateLimitedError(time.Second, testKey) inner := fmt.Errorf("decode %s: %w", testKey, llm.ErrBadOutput) - joined := errors.Join(inner, pe, rl, context.Canceled, &transportFailure{detail: "slow", kind: llm.ErrTimeout}, fmt.Errorf("HTTP 401: %w", llm.ErrAuth)) + joined := errors.Join(inner, pe, rl, context.Canceled, &transportFailure{text: "llm: request timed out: transport: slow", kind: llm.ErrTimeout}, fmt.Errorf("HTTP 401: %w", llm.ErrAuth)) err := redact(fmt.Errorf("anthropic %s: %w", testKey, joined), testKey) assertChainNoExactKey(t, err, testKey) for _, target := range []error{llm.ErrBadOutput, llm.ErrProvider, llm.ErrRateLimited, llm.ErrTimeout, llm.ErrAuth, context.Canceled} { @@ -886,8 +886,8 @@ func TestRedactKeepsChainMatches(t *testing.T) { t.Fatalf("errors.As lost a typed error in %v", err) } clean := errors.New("anthropic: generate: no model configured") - if redact(clean, testKey) != clean { - t.Fatal("redact wrapped an error that holds no key") + if got := redact(clean, testKey); got.Error() != clean.Error() || !errors.Is(got, clean) { + t.Fatalf("redact(%v) = %v, want the same text and chain", clean, got) } } @@ -970,3 +970,56 @@ func TestRedirectIsNotFollowed(t *testing.T) { t.Fatalf("redirect target got %d requests, want 0", secondCalls) } } + +// assertGoStringNoKey checks that %#v of err, of every layer reached by +// unwrapping and of the typed errors reached by errors.As holds no key. +func assertGoStringNoKey(t *testing.T, err error, key string) { + t.Helper() + var walk func(error) + walk = func(e error) { + if e == nil { + return + } + if got := fmt.Sprintf("%#v", e); strings.Contains(got, key) { + t.Fatalf("%%#v of %T leaks %q: %q", e, key, got) + } + switch u := e.(type) { + case interface{ Unwrap() error }: + walk(u.Unwrap()) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + walk(c) + } + } + } + walk(err) + var pe *llm.ProviderError + if errors.As(err, &pe) { + walk(pe) + } + var rl *llm.RateLimitedError + if errors.As(err, &rl) { + walk(rl) + } +} + +// TestFieldNameKeyNeverLeaksThroughGoString uses struct field names of the +// returned error types as the API key; %#v must not print any of them. +func TestFieldNameKeyNeverLeaksThroughGoString(t *testing.T) { + for _, key := range []string{"body", "status", "text", "RetryAfter", "msg", "err", "key", "kind"} { + vs := newVendorServerWithKey(t, key, reply{status: 500, body: "down"}, reply{status: 429, header: map[string]string{"Retry-After": "17"}, body: "slow"}) + p := newProviderWithKey(t, vs.URL, key) + _, err := p.Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.As(err, &pe) || pe.Status() != 500 { + t.Fatalf("key %q: err = %v, want a 500 ProviderError", key, err) + } + assertGoStringNoKey(t, err, key) + _, err = p.Generate(context.Background(), schemaRequest()) + var rl *llm.RateLimitedError + if !errors.As(err, &rl) || rl.RetryAfter != 17*time.Second { + t.Fatalf("key %q: err = %v, want a RateLimitedError with RetryAfter 17s", key, err) + } + assertGoStringNoKey(t, err, key) + } +} diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index 07fddef..e4f1bd2 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -58,6 +58,9 @@ func (e *RateLimitedError) Error() string { return fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter) } +// GoString keeps %#v to the redacted Error text instead of the fields. +func (e *RateLimitedError) GoString() string { return e.Error() } + // Is reports whether target is ErrRateLimited. func (e *RateLimitedError) Is(target error) bool { return target == ErrRateLimited } @@ -128,5 +131,9 @@ func (e *ProviderError) Body() string { return e.body } func (e *ProviderError) Error() string { return e.text } +// GoString keeps %#v to the redacted Error text instead of the fields, +// whose names could spell the credential. +func (e *ProviderError) GoString() string { return e.text } + // Is reports whether target is ErrProvider. func (e *ProviderError) Is(target error) bool { return target == ErrProvider } diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index a467e9b..4f21be7 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -505,16 +505,14 @@ func isTimeout(err error) bool { // with the request URL removed and the API key redacted, and unwraps only to // the cause's category: llm.ErrTimeout, context.Canceled or nothing. type transportFailure struct { - detail string - kind error + text string + kind error } -func (e *transportFailure) Error() string { - if e.kind == llm.ErrTimeout { - return fmt.Sprintf("%s: transport: %s", llm.ErrTimeout, e.detail) - } - return "transport: " + e.detail -} +func (e *transportFailure) Error() string { return e.text } + +// GoString keeps %#v to the redacted Error text instead of the fields. +func (e *transportFailure) GoString() string { return e.text } func (e *transportFailure) Unwrap() error { return e.kind } @@ -533,7 +531,11 @@ func (p *Provider) transportError(err error) error { if errors.As(err, &ue) { err = ue.Err } - return &transportFailure{detail: llm.Redact(err.Error(), p.apiKey), kind: kind} + text := "transport: " + err.Error() + if kind == llm.ErrTimeout { + text = fmt.Sprintf("%s: %s", llm.ErrTimeout, text) + } + return &transportFailure{text: llm.Redact(text, p.apiKey), kind: kind} } // redactedError stands in for one layer of an error chain that holds the @@ -542,8 +544,8 @@ func (p *Provider) transportError(err error) error { // unwrapping has text holding the key. Is consults the original layer and // its chain, so sentinels still match. As does too. The llm typed errors // remove the key from their own complete text, fixed words included, so -// they always match; As refuses only a match from outside llm whose text -// holds the key, so it never hands back an unredacted error. +// they always match; As refuses a match whose text, or whose %#v form, +// holds the key, so it never hands back an error that can print it. type redactedError struct { text string err error @@ -564,7 +566,8 @@ func (e *redactedError) As(target any) bool { return false } found := reflect.ValueOf(target).Elem() - if matched, ok := found.Interface().(error); ok && holdsKey(matched, e.key) { + matched, isErr := found.Interface().(error) + if (isErr && holdsKey(matched, e.key)) || strings.Contains(fmt.Sprintf("%#v", found.Interface()), e.key) { found.SetZero() return false } @@ -586,12 +589,13 @@ func (e *redactedJoin) Unwrap() []error { return safe } -// redact is the backstop every exported method applies to its error: when -// the text of err or of any error in its chain holds a non-empty key, it -// returns a stand-in whose text, and the text of every error it unwraps -// to, has the key redacted. Any other err is returned unchanged. +// redact is the backstop every exported method applies to its error: with +// a non-empty key, it returns a stand-in whose text, and the text of every +// error it unwraps to, has the key redacted. Every non-nil err is wrapped, +// even one whose text lacks the key, because %#v of an unwrapped error +// prints its struct field names, and a key may spell one. func redact(err error, key string) error { - if err == nil || key == "" || !holdsKey(err, key) { + if err == nil || key == "" { return err } re := redactedError{text: llm.Redact(err.Error(), key), err: err, key: key} diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 23d4c16..c953fe1 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -885,8 +885,8 @@ func TestRedactKeepsChainAndHidesKey(t *testing.T) { t.Fatal("redact changed an error for the empty key") } clean := errors.New("openai: generate: no model configured") - if redact(clean, testKey) != clean { - t.Fatal("redact wrapped an error that holds no key") + if got := redact(clean, testKey); got.Error() != clean.Error() || !errors.Is(got, clean) { + t.Fatalf("redact(%v) = %v, want the same text and chain", clean, got) } // A key inside the redaction marker would survive replacement. if got := redact(errors.New("x E y"), "E").Error(); strings.Contains(got, "E") { @@ -941,7 +941,7 @@ func TestRedactMakesEveryUnwrappedLayerSafe(t *testing.T) { fmt.Errorf("openai: generate: %w", inner), fmt.Errorf("openai: generate: %w", hidden), fmt.Errorf("openai: generate: %w", joined), - fmt.Errorf("openai: %w and %w", inner, &transportFailure{detail: "slow", kind: llm.ErrTimeout}), + fmt.Errorf("openai: %w and %w", inner, &transportFailure{text: "llm: request timed out: transport: slow", kind: llm.ErrTimeout}), } for _, c := range cases { err := redact(c, testKey) @@ -967,7 +967,7 @@ func TestRedactMakesEveryUnwrappedLayerSafe(t *testing.T) { if !errors.Is(err, llm.ErrAuth) { t.Fatalf("errors.Is lost ErrAuth in %v", err) } - err = redact(fmt.Errorf("openai %s: %w", testKey, &transportFailure{detail: "slow", kind: llm.ErrTimeout}), testKey) + err = redact(fmt.Errorf("openai %s: %w", testKey, &transportFailure{text: "llm: request timed out: transport: slow", kind: llm.ErrTimeout}), testKey) if !errors.Is(err, llm.ErrTimeout) { t.Fatalf("errors.Is lost ErrTimeout in %v", err) } @@ -1202,3 +1202,54 @@ func TestRedirectIsNotFollowed(t *testing.T) { t.Fatalf("redirect target got %d requests, want 0", secondCalls) } } + +// assertGoStringNoKey checks that %#v of err, of every layer reached by +// unwrapping and of the typed errors reached by errors.As holds no key. +func assertGoStringNoKey(t *testing.T, err error, key string) { + t.Helper() + var walk func(error) + walk = func(e error) { + if e == nil { + return + } + if got := fmt.Sprintf("%#v", e); strings.Contains(got, key) { + t.Fatalf("%%#v of %T leaks %q: %q", e, key, got) + } + switch u := e.(type) { + case interface{ Unwrap() error }: + walk(u.Unwrap()) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + walk(c) + } + } + } + walk(err) + var pe *llm.ProviderError + if errors.As(err, &pe) { + walk(pe) + } + var rl *llm.RateLimitedError + if errors.As(err, &rl) { + walk(rl) + } +} + +// TestFieldNameKeyNeverLeaksThroughGoString uses struct field names of the +// returned error types as the API key; %#v must not print any of them. +func TestFieldNameKeyNeverLeaksThroughGoString(t *testing.T) { + for _, key := range []string{"body", "status", "text", "RetryAfter", "msg", "err", "key", "kind"} { + _, err := keyServerWithKey(t, key, 500, "down").Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.As(err, &pe) || pe.Status() != 500 { + t.Fatalf("key %q: err = %v, want a 500 ProviderError", key, err) + } + assertGoStringNoKey(t, err, key) + err = keyServerWithKey(t, key, 429, "slow").Verify(context.Background()) + var rl *llm.RateLimitedError + if !errors.As(err, &rl) { + t.Fatalf("key %q: err = %v, want a RateLimitedError", key, err) + } + assertGoStringNoKey(t, err, key) + } +}