diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index 555b99a..f826dfc 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -46,8 +46,27 @@ func (e *RateLimitedError) Is(target error) bool { return target == ErrRateLimit // ProviderError keeps. const MaxBodyExcerpt = 512 -// redacted replaces each secret in a ProviderError body. -const redacted = "[REDACTED]" +// redactionMarker replaces each secret in redacted text, unless the marker +// would itself hold the secret. +const redactionMarker = "[REDACTED]" + +// Redact returns s with every occurrence of a non-empty secret removed. Each +// occurrence becomes "[REDACTED]" when the result then holds no secret; +// otherwise, because the marker contains the secret or joins with its +// neighbours to form it, occurrences are deleted until none remains. The +// result never contains secret. +func Redact(s, secret string) string { + if secret == "" { + return s + } + if out := strings.ReplaceAll(s, secret, redactionMarker); !strings.Contains(out, secret) { + return out + } + for strings.Contains(s, secret) { + s = strings.ReplaceAll(s, secret, "") + } + return s +} // ProviderError is an unexpected HTTP status from the vendor. It matches // ErrProvider. Build it with NewProviderError, which redacts the body. @@ -59,20 +78,17 @@ type ProviderError struct { // NewProviderError returns the error for an unexpected HTTP status. It // always keeps a body excerpt of at most MaxBodyExcerpt bytes. secret is the // credential sent with the request, or "" when the request sent none; every -// occurrence of a non-empty secret in body is replaced before the body is -// cut, so no part of it survives. +// occurrence of a non-empty secret in body is removed by Redact before the +// body is cut, so no part of it survives, and again after the cut, so the +// truncation mark cannot complete it. func NewProviderError(status int, body []byte, secret string) *ProviderError { - s := string(body) - if secret != "" { - s = strings.ReplaceAll(s, secret, redacted) - } - s = strings.TrimSpace(s) + s := strings.TrimSpace(Redact(string(body), secret)) if len(s) > MaxBodyExcerpt { cut := MaxBodyExcerpt for cut > 0 && !utf8.RuneStart(s[cut]) { cut-- } - s = s[:cut] + "…" + s = Redact(s[:cut]+"…", secret) } return &ProviderError{status: status, body: s} } diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index 8911611..b08d632 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -117,3 +117,45 @@ func TestProviderErrorKeepsBoundedBodyWithoutSecret(t *testing.T) { t.Fatalf("Body() has %d bytes, want %d", len(long.Body()), len(want)) } } + +func TestRedactNeverLeavesSecret(t *testing.T) { + cases := []struct{ s, secret, want string }{ + {"key sk-1 and Bearer sk-1", "sk-1", "key [REDACTED] and Bearer [REDACTED]"}, + {"no secret here", "sk-1", "no secret here"}, + {"x", "", "x"}, + // The marker holds the secret, so occurrences are deleted. + {"rejected REDACTED", "REDACTED", "rejected "}, + {"A then A", "A", " then "}, + // The marker joins with a neighbour to form the secret. + {"]]aa", "]a", ""}, + // The marker holds the secret and deleting once leaves a new one. + {"DDAA", "DA", ""}, + {"aabb", "ab", "a[REDACTED]b"}, + } + for _, c := range cases { + if got := Redact(c.s, c.secret); got != c.want { + t.Errorf("Redact(%q, %q) = %q, want %q", c.s, c.secret, got, c.want) + } + } +} + +func TestProviderErrorWithMarkerSecretNeverLeaksSecret(t *testing.T) { + const key = "REDACTED" + pe := NewProviderError(500, []byte(`{"error":"bad key REDACTED","echo":"Bearer REDACTED"}`), key) + for _, s := range []string{pe.Error(), pe.Body()} { + if strings.Contains(s, key) { + t.Fatalf("secret leaked: %q", s) + } + } + if want := `{"error":"bad key ","echo":"Bearer "}`; pe.Body() != want { + t.Fatalf("Body() = %q, want %q", pe.Body(), want) + } +} + +func TestProviderErrorTruncationMarkCannotCompleteSecret(t *testing.T) { + const key = "z…" + body := strings.Repeat("a", MaxBodyExcerpt-1) + "z" + strings.Repeat("b", 10) + if pe := NewProviderError(500, []byte(body), key); strings.Contains(pe.Body(), key) { + t.Fatalf("secret leaked: %q", pe.Body()) + } +} diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 785d1ad..52fd89d 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -19,6 +19,7 @@ import ( "net/http" "net/textproto" "net/url" + "reflect" "strconv" "strings" "time" @@ -510,27 +511,15 @@ func (p *Provider) transportError(err error) error { if errors.As(err, &ue) { err = ue.Err } - return &transportFailure{detail: redactKey(err.Error(), p.apiKey), kind: kind} + return &transportFailure{detail: llm.Redact(err.Error(), p.apiKey), kind: kind} } -// redactKey replaces every occurrence of a non-empty key in s. Replacing -// the key also replaces the "Bearer "+key header value that contains it. -func redactKey(s, key string) string { - if key == "" { - return s - } - return strings.ReplaceAll(s, key, "[REDACTED]") -} - -// withheldText replaces an error's text when redactKey cannot remove the -// key, because the key is part of the replacement marker. -const withheldText = "openai: error text withheld: it contains the API key" - // 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. +// unwrapping has text holding the key. Is consults the original layer and +// its chain, so sentinels still match. As does too, but refuses a match +// whose text holds the key, so it never hands back an unredacted error. type redactedError struct { text string err error @@ -543,7 +532,17 @@ func (e *redactedError) Unwrap() error { return redact(errors.Unwrap(e.err), e.k 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) } +func (e *redactedError) As(target any) bool { + if !errors.As(e.err, target) { + return false + } + found := reflect.ValueOf(target).Elem() + if matched, ok := found.Interface().(error); ok && holdsKey(matched, e.key) { + found.SetZero() + return false + } + return true +} // 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. @@ -568,11 +567,7 @@ func redact(err error, key string) error { if err == nil || key == "" || !holdsKey(err, key) { return err } - text := redactKey(err.Error(), key) - if strings.Contains(text, key) { - text = withheldText - } - re := redactedError{text: text, err: err, key: key} + re := redactedError{text: llm.Redact(err.Error(), key), err: err, key: key} if _, ok := err.(interface{ Unwrap() []error }); ok { return &redactedJoin{re} } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index dea2b44..ca734ad 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -803,6 +803,12 @@ func assertNoKey(t *testing.T, err error, key string) { // keyServer replies to every request with status and body. func keyServer(t *testing.T, status int, body string) *Provider { + t.Helper() + return keyServerWithKey(t, headerNameKey, status, body) +} + +// keyServerWithKey is keyServer for a provider configured with key. +func keyServerWithKey(t *testing.T, key string, status int, body string) *Provider { t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -810,7 +816,7 @@ func keyServer(t *testing.T, status int, body string) *Provider { io.WriteString(w, body) })) t.Cleanup(srv.Close) - p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: headerNameKey, Model: "test-model", StructuredOutput: PromptOnly}) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: key, Model: "test-model", StructuredOutput: PromptOnly}) if err != nil { t.Fatal(err) } @@ -970,3 +976,69 @@ type opaqueError struct{ err error } func (e *opaqueError) Error() string { return "opaque" } func (e *opaqueError) Unwrap() error { return e.err } + +// assertChainNoExactKey fails if the text of err, of any error reached from +// it by unwrapping, or of a *llm.ProviderError found by errors.As holds key. +// It matches case-sensitively, so it suits keys too short for assertNoKey. +func assertChainNoExactKey(t *testing.T, err error, key string) { + t.Helper() + var walk func(error) + walk = func(e error) { + if e == nil { + return + } + if strings.Contains(e.Error(), key) { + t.Fatalf("error leaks %q: %q", key, e.Error()) + } + 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) && (strings.Contains(pe.Body(), key) || strings.Contains(pe.Error(), key)) { + t.Fatalf("ProviderError leaks %q: body %q", key, pe.Body()) + } + var syntax *json.SyntaxError + if errors.As(err, &syntax) && strings.Contains(syntax.Error(), key) { + t.Fatalf("errors.As reached a *json.SyntaxError leaking %q: %q", key, syntax.Error()) + } +} + +// TestProviderErrorBodyWithMarkerKeyNeverLeaksKey uses the redaction marker +// text as the API key, so replacing the key with the marker would keep it. +func TestProviderErrorBodyWithMarkerKeyNeverLeaksKey(t *testing.T) { + const key = "REDACTED" + p := keyServerWithKey(t, key, 500, `{"error":{"message":"rejected `+key+`: Bearer `+key+`"}}`) + _, err := p.Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.Is(err, llm.ErrProvider) || !errors.As(err, &pe) || pe.Status() != 500 { + t.Fatalf("err = %v, want a 500 ProviderError", err) + } + if strings.Contains(pe.Body(), key) || strings.Contains(pe.Error(), key) { + t.Fatalf("ProviderError leaks the key: %q", pe.Error()) + } + assertChainNoExactKey(t, err, key) +} + +// TestGenerateMalformedJSONWithShortKeyNeverLeaksKey uses a one-letter key +// found in the redaction marker and in ordinary error prose. +func TestGenerateMalformedJSONWithShortKeyNeverLeaksKey(t *testing.T) { + const key = "A" + p := keyServerWithKey(t, key, 200, `{"choices": A Bearer A}`) + _, err := p.Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate accepted a malformed JSON body") + } + assertChainNoExactKey(t, err, key) + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + if strings.Contains(layer.Error(), key) { + t.Fatalf("layer leaks %q: %q", key, layer.Error()) + } + } +}