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}
 }

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())
+	}
+}

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}
 	}

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())
+		}
+	}
+}