From cceeafd20076cdd45e294385e3f9a351c148506d Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:26:37 -0400 Subject: [PATCH] Redact every layer of a returned error chain, not just the top redactedError.Unwrap returned the original error, so errors.Unwrap could reach an inner error whose text still held the API key. Unwrap now passes each cause through redact, so every error reached by unwrapping has the key removed, and errors with several causes are handled the same way. redact also checks the whole chain, catching a layer that hides the key from its own text but wraps an error that shows it. Is and As consult the original chain, so sentinels and *llm.ProviderError still match. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 70 +++++++++++++++++---- internal/ai/llm/openai/openai_test.go | 90 ++++++++++++++++++++++++++- 2 files changed, 146 insertions(+), 14 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index cc4c13f..785d1ad 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -526,31 +526,77 @@ func redactKey(s, key string) string { // key, because the key is part of the replacement marker. const withheldText = "openai: error text withheld: it contains the API key" -// redactedError is an error whose text had the API key removed. It unwraps -// to the original error, so errors.Is and errors.As still see the chain. +// redactedError stands in for one layer of an error chain that holds the +// API key. Its text is the layer's text with the key redacted, and Unwrap +// returns the layer's causes passed through redact, so no error reached by +// unwrapping has text holding the key. Is and As consult the original +// layer and its chain, so sentinels and typed errors still match. type redactedError struct { text string err error + key string } func (e *redactedError) Error() string { return e.text } -func (e *redactedError) Unwrap() error { return e.err } +func (e *redactedError) Unwrap() error { return redact(errors.Unwrap(e.err), e.key) } + +func (e *redactedError) Is(target error) bool { return errors.Is(e.err, target) } + +func (e *redactedError) As(target any) bool { return errors.As(e.err, target) } + +// redactedJoin is redactedError for a layer with several causes, such as +// one built by errors.Join or by fmt.Errorf with more than one %w. +type redactedJoin struct { + redactedError +} + +func (e *redactedJoin) Unwrap() []error { + causes := e.err.(interface{ Unwrap() []error }).Unwrap() + safe := make([]error, len(causes)) + for i, c := range causes { + safe[i] = redact(c, e.key) + } + return safe +} // redact is the backstop every exported method applies to its error: when -// err's text, wrapped errors included, holds a non-empty key, it returns err -// with that text redacted. Any other err is returned unchanged. +// 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. func redact(err error, key string) error { - if err == nil || key == "" { + if err == nil || key == "" || !holdsKey(err, key) { return err } - text := err.Error() - if !strings.Contains(text, key) { - return err - } - text = redactKey(text, key) + text := redactKey(err.Error(), key) if strings.Contains(text, key) { text = withheldText } - return &redactedError{text: text, err: err} + re := redactedError{text: text, err: err, key: key} + if _, ok := err.(interface{ Unwrap() []error }); ok { + return &redactedJoin{re} + } + return &re +} + +// holdsKey reports whether the text of err or of any error in its chain +// contains key. +func holdsKey(err error, key string) bool { + if err == nil { + return false + } + if strings.Contains(err.Error(), key) { + return true + } + switch u := err.(type) { + case interface{ Unwrap() error }: + return holdsKey(u.Unwrap(), key) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + if holdsKey(c, key) { + return true + } + } + } + return false } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index cc45f32..dea2b44 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -868,8 +868,8 @@ func TestProviderErrorBodyNeverLeaksKey(t *testing.T) { func TestRedactKeepsChainAndHidesKey(t *testing.T) { inner := fmt.Errorf("decode: body %q: %w", "Bearer "+testKey, llm.ErrBadOutput) err := redact(inner, testKey) - assertNoSecret(t, err) - if !errors.Is(err, llm.ErrBadOutput) || errors.Unwrap(err) != inner { + assertChainNoKey(t, err, testKey) + if !errors.Is(err, llm.ErrBadOutput) { t.Fatalf("redact broke the chain of %v", err) } if redact(inner, "") != inner { @@ -884,3 +884,89 @@ func TestRedactKeepsChainAndHidesKey(t *testing.T) { t.Fatalf("redact kept the key: %q", got) } } + +// assertChainNoKey fails if err, or any error reached from it by unwrapping, +// has text holding key or "Bearer "+key. +func assertChainNoKey(t *testing.T, err error, key string) { + t.Helper() + if err == nil { + return + } + assertNoKey(t, err, key) + switch u := err.(type) { + case interface{ Unwrap() error }: + assertChainNoKey(t, u.Unwrap(), key) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + assertChainNoKey(t, c, key) + } + } +} + +// TestGenerateInvalidJSONWithKeyNeverLeaksKeyByUnwrapping sends an invalid +// JSON body holding the key and walks the returned chain with errors.Unwrap. +func TestGenerateInvalidJSONWithKeyNeverLeaksKeyByUnwrapping(t *testing.T) { + p := keyServer(t, 200, `{"choices": Bearer `+headerNameKey+` `+headerNameKey+`}`) + _, err := p.Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate accepted an invalid JSON body") + } + var syntax *json.SyntaxError + if !errors.As(err, &syntax) { + t.Fatalf("err = %v, want a *json.SyntaxError in the chain", err) + } + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + assertNoKey(t, layer, headerNameKey) + } +} + +// TestRedactMakesEveryUnwrappedLayerSafe covers chains whose inner layers +// hold the key, including a layer whose own text does not show it and a +// layer with several causes, and checks Is and As still match. +func TestRedactMakesEveryUnwrappedLayerSafe(t *testing.T) { + pe := llm.NewProviderError(500, []byte(`{"error":"down"}`), testKey) + hidden := &opaqueError{err: fmt.Errorf("body Bearer %s: %w", testKey, llm.ErrAuth)} + inner := fmt.Errorf("decode %s: %w", testKey, llm.ErrBadOutput) + joined := errors.Join(inner, pe, context.Canceled, fmt.Errorf("limit: %w", llm.ErrRateLimited)) + cases := []error{ + 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}), + } + for _, c := range cases { + err := redact(c, testKey) + if err == c { + t.Fatalf("redact left %v unchanged", c) + } + assertChainNoKey(t, err, testKey) + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + assertNoKey(t, layer, testKey) + } + } + err := redact(fmt.Errorf("openai: %w", joined), testKey) + var got *llm.ProviderError + if !errors.As(err, &got) || got != pe { + t.Fatalf("errors.As lost the ProviderError in %v", err) + } + for _, target := range []error{llm.ErrBadOutput, llm.ErrProvider, llm.ErrRateLimited, context.Canceled} { + if !errors.Is(err, target) { + t.Fatalf("errors.Is(%v, %v) = false", err, target) + } + } + err = redact(fmt.Errorf("openai: %w", hidden), testKey) + 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) + if !errors.Is(err, llm.ErrTimeout) { + t.Fatalf("errors.Is lost ErrTimeout in %v", err) + } +} + +// opaqueError hides its cause's text but unwraps to it. +type opaqueError struct{ err error } + +func (e *opaqueError) Error() string { return "opaque" } + +func (e *opaqueError) Unwrap() error { return e.err }