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 }