diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index f826dfc..50183e6 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -26,17 +26,28 @@ var ( ) // RateLimitedError is a throttled request. It matches ErrRateLimited. +// Adapters build it with NewRateLimitedError so its text never holds their +// credential. type RateLimitedError struct { // RetryAfter is how long the vendor asked callers to wait; 0 means it // did not say. RetryAfter time.Duration + secret string +} + +// NewRateLimitedError returns the error for a throttled request. secret is +// the credential sent with the request, or "" when the request sent none; +// Error removes every occurrence of a non-empty secret, even where the +// fixed text happens to spell it. +func NewRateLimitedError(retryAfter time.Duration, secret string) *RateLimitedError { + return &RateLimitedError{RetryAfter: retryAfter, secret: secret} } func (e *RateLimitedError) Error() string { if e.RetryAfter == 0 { - return ErrRateLimited.Error() + return Redact(ErrRateLimited.Error(), e.secret) } - return fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter) + return Redact(fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter), e.secret) } // Is reports whether target is ErrRateLimited. @@ -73,6 +84,7 @@ func Redact(s, secret string) string { type ProviderError struct { status int body string + secret string } // NewProviderError returns the error for an unexpected HTTP status. It @@ -80,7 +92,8 @@ type ProviderError struct { // credential sent with the request, or "" when the request sent none; every // 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. +// truncation mark cannot complete it. Error removes it from the complete +// text too, even where the fixed text happens to spell it. func NewProviderError(status int, body []byte, secret string) *ProviderError { s := strings.TrimSpace(Redact(string(body), secret)) if len(s) > MaxBodyExcerpt { @@ -90,7 +103,7 @@ func NewProviderError(status int, body []byte, secret string) *ProviderError { } s = Redact(s[:cut]+"…", secret) } - return &ProviderError{status: status, body: s} + return &ProviderError{status: status, body: s, secret: secret} } // Status returns the HTTP status code. @@ -102,9 +115,9 @@ func (e *ProviderError) Body() string { return e.body } func (e *ProviderError) Error() string { if e.body == "" { - return fmt.Sprintf("%s: HTTP %d", ErrProvider, e.status) + return Redact(fmt.Sprintf("%s: HTTP %d", ErrProvider, e.status), e.secret) } - return fmt.Sprintf("%s: HTTP %d: %s", ErrProvider, e.status, e.body) + return Redact(fmt.Sprintf("%s: HTTP %d: %s", ErrProvider, e.status, e.body), e.secret) } // Is reports whether target is ErrProvider. diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index b08d632..b5784ad 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -159,3 +159,28 @@ func TestProviderErrorTruncationMarkCannotCompleteSecret(t *testing.T) { t.Fatalf("secret leaked: %q", pe.Body()) } } + +func TestProviderErrorWithFixedWordSecretNeverLeaksSecret(t *testing.T) { + const secret = "HTTP" + pe := NewProviderError(500, []byte(`{"error":"HTTP down"}`), secret) + if strings.Contains(pe.Error(), secret) || strings.Contains(pe.Body(), secret) { + t.Fatalf("secret leaked: Error %q, Body %q", pe.Error(), pe.Body()) + } + if got := NewProviderError(500, nil, secret).Error(); got != "llm: provider error: [REDACTED] 500" { + t.Fatalf("Error() without body = %q", got) + } +} + +func TestRateLimitedErrorWithFixedWordSecretNeverLeaksSecret(t *testing.T) { + for _, secret := range []string{"rate", "retry after"} { + for _, wait := range []time.Duration{0, 30 * time.Second} { + rl := NewRateLimitedError(wait, secret) + if strings.Contains(rl.Error(), secret) { + t.Fatalf("secret %q leaked: %q", secret, rl.Error()) + } + if rl.RetryAfter != wait || !errors.Is(rl, ErrRateLimited) { + t.Fatalf("NewRateLimitedError(%v) = %+v", wait, rl) + } + } + } +} diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 52fd89d..e6493de 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -446,7 +446,7 @@ func (p *Provider) statusError(resp *http.Response, body []byte) error { case http.StatusUnauthorized, http.StatusForbidden: return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrAuth) case http.StatusTooManyRequests: - return &llm.RateLimitedError{RetryAfter: retryAfter(resp.Header.Get("Retry-After"), time.Now())} + return llm.NewRateLimitedError(retryAfter(resp.Header.Get("Retry-After"), time.Now()), p.apiKey) case http.StatusRequestTimeout: return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) default: @@ -518,8 +518,10 @@ func (p *Provider) transportError(err error) error { // 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 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. +// 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. type redactedError struct { text string err error diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index ca734ad..8f0a248 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -1042,3 +1042,80 @@ func TestGenerateMalformedJSONWithShortKeyNeverLeaksKey(t *testing.T) { } } } + +// assertLayersNoExactKey fails if the text of err or of any error reached +// from it by errors.Unwrap holds key, matched case-sensitively. +func assertLayersNoExactKey(t *testing.T, err error, key string) { + t.Helper() + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + if strings.Contains(layer.Error(), key) { + t.Fatalf("layer leaks %q: %q", key, layer.Error()) + } + } +} + +// TestProviderErrorWithFixedWordKeyKeepsTypedMatch uses the fixed word +// "HTTP" of ProviderError's text as the API key: the text must drop it and +// errors.As must still reach the ProviderError. +func TestProviderErrorWithFixedWordKeyKeepsTypedMatch(t *testing.T) { + const key = "HTTP" + p := keyServerWithKey(t, key, 500, `{"error":"down"}`) + _, 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.Error(), key) || strings.Contains(pe.Body(), key) { + t.Fatalf("ProviderError leaks the key: Error %q, Body %q", pe.Error(), pe.Body()) + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) +} + +// TestRateLimitedErrorWithFixedWordKeyKeepsTypedMatch uses the fixed word +// "rate" of RateLimitedError's text as the API key. +func TestRateLimitedErrorWithFixedWordKeyKeepsTypedMatch(t *testing.T) { + const key = "rate" + srv := newVendorServer(t, reply{status: 429, header: map[string]string{"Retry-After": "17"}}) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: key, Model: "test-model", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + _, err = p.Generate(context.Background(), schemaRequest()) + var rl *llm.RateLimitedError + if !errors.Is(err, llm.ErrRateLimited) || !errors.As(err, &rl) || rl.RetryAfter != 17*time.Second { + t.Fatalf("err = %v, want a RateLimitedError with RetryAfter 17s", err) + } + if strings.Contains(rl.Error(), key) { + t.Fatalf("RateLimitedError leaks the key: %q", rl.Error()) + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) +} + +// TestGenerateWithAdapterNameKeyNeverLeaksKey uses the fixed prefix +// "openai" of the adapter's errors as the API key. +func TestGenerateWithAdapterNameKeyNeverLeaksKey(t *testing.T) { + const key = "openai" + t.Run("401", func(t *testing.T) { + p := keyServerWithKey(t, key, 401, `{"error":{"message":"bad key"}}`) + _, err := p.Generate(context.Background(), schemaRequest()) + if !errors.Is(err, llm.ErrAuth) { + t.Fatalf("err = %v, want ErrAuth", err) + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) + }) + t.Run("no model", func(t *testing.T) { + p, err := New(Config{BaseURL: "http://127.0.0.1:1/v1", APIKey: key, StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + _, err = p.Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate succeeded without a model") + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) + }) +}