diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index be8e922..555b99a 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -56,23 +56,15 @@ type ProviderError struct { body string } -// NewProviderError returns the error for an unexpected HTTP status. -// secrets must hold the API key (and any other credential sent with the -// request). Every occurrence of each non-empty secret in body is replaced -// before the body is cut to MaxBodyExcerpt bytes, so no part of a secret -// survives. If secrets holds no non-empty value the body cannot be -// redacted, so no excerpt is kept at all. -func NewProviderError(status int, body []byte, secrets []string) *ProviderError { +// 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. +func NewProviderError(status int, body []byte, secret string) *ProviderError { s := string(body) - redactedAny := false - for _, secret := range secrets { - if secret != "" { - s = strings.ReplaceAll(s, secret, redacted) - redactedAny = true - } - } - if !redactedAny { - return &ProviderError{status: status} + if secret != "" { + s = strings.ReplaceAll(s, secret, redacted) } s = strings.TrimSpace(s) if len(s) > MaxBodyExcerpt { @@ -89,7 +81,7 @@ func NewProviderError(status int, body []byte, secrets []string) *ProviderError func (e *ProviderError) Status() int { return e.status } // Body returns the redacted, truncated response-body excerpt; it is empty -// when the vendor sent no body or no secret was given to redact it with. +// when the vendor sent no body. func (e *ProviderError) Body() string { return e.body } func (e *ProviderError) Error() string { diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index 321f887..8911611 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -46,7 +46,7 @@ func TestRateLimitedError(t *testing.T) { } func TestProviderError(t *testing.T) { - err := fmt.Errorf("gemini: %w", NewProviderError(503, []byte(` {"error":"overloaded"} `), []string{"sk-test"})) + err := fmt.Errorf("gemini: %w", NewProviderError(503, []byte(` {"error":"overloaded"} `), "sk-test")) if !errors.Is(err, ErrProvider) { t.Fatal("errors.Is(err, ErrProvider) = false") } @@ -63,7 +63,7 @@ func TestProviderError(t *testing.T) { if got := err.Error(); got != `gemini: llm: provider error: HTTP 503: {"error":"overloaded"}` { t.Fatalf("Error() = %q", got) } - if got := NewProviderError(500, nil, []string{"sk-test"}).Error(); got != "llm: provider error: HTTP 500" { + if got := NewProviderError(500, nil, "sk-test").Error(); got != "llm: provider error: HTTP 500" { t.Fatalf("Error() without body = %q", got) } } @@ -71,7 +71,7 @@ func TestProviderError(t *testing.T) { func TestProviderErrorRedactsSecrets(t *testing.T) { const key = "sk-live-0123456789abcdef" body := []byte(`{"error":"invalid key sk-live-0123456789abcdef","echo":"Bearer sk-live-0123456789abcdef"}`) - pe := NewProviderError(400, body, []string{key, ""}) + pe := NewProviderError(400, body, key) for _, s := range []string{pe.Error(), pe.Body()} { if strings.Contains(s, key) { t.Fatalf("secret leaked: %q", s) @@ -88,7 +88,7 @@ func TestProviderErrorRedactsBeforeTruncating(t *testing.T) { // The key starts just before the excerpt limit, so truncating first // would leave its prefix in the excerpt. body := strings.Repeat("x", MaxBodyExcerpt-5) + key - pe := NewProviderError(500, []byte(body), []string{key}) + pe := NewProviderError(500, []byte(body), key) if strings.Contains(pe.Body(), "sk-se") { t.Fatalf("secret prefix leaked: %q", pe.Body()) } @@ -96,26 +96,24 @@ func TestProviderErrorRedactsBeforeTruncating(t *testing.T) { func TestProviderErrorTruncatesOnRuneBoundary(t *testing.T) { body := strings.Repeat("a", MaxBodyExcerpt-1) + "é" + strings.Repeat("b", 100) - pe := NewProviderError(500, []byte(body), []string{"sk-test"}) + pe := NewProviderError(500, []byte(body), "sk-test") want := strings.Repeat("a", MaxBodyExcerpt-1) + "…" if pe.Body() != want { t.Fatalf("Body() has %d bytes, want %d", len(pe.Body()), len(want)) } } -func TestProviderErrorOmitsBodyWithoutSecret(t *testing.T) { - const key = "sk-live-0123456789abcdef" - body := []byte(`{"error":"invalid key sk-live-0123456789abcdef"}`) - for _, secrets := range [][]string{nil, {}, {""}} { - pe := NewProviderError(401, body, secrets) - if pe.Body() != "" { - t.Fatalf("secrets %q: Body() = %q, want empty", secrets, pe.Body()) - } - if got := pe.Error(); got != "llm: provider error: HTTP 401" { - t.Fatalf("secrets %q: Error() = %q", secrets, got) - } - if strings.Contains(pe.Error(), key) { - t.Fatalf("secrets %q: secret leaked: %q", secrets, pe.Error()) - } +func TestProviderErrorKeepsBoundedBodyWithoutSecret(t *testing.T) { + body := []byte(` {"error":"model \"llama9\" not found"} `) + pe := NewProviderError(404, body, "") + if pe.Body() != `{"error":"model \"llama9\" not found"}` { + t.Fatalf("Body() = %q", pe.Body()) + } + if got := pe.Error(); got != `llm: provider error: HTTP 404: {"error":"model \"llama9\" not found"}` { + t.Fatalf("Error() = %q", got) + } + long := NewProviderError(500, []byte(strings.Repeat("x", 4*MaxBodyExcerpt)), "") + if want := strings.Repeat("x", MaxBodyExcerpt) + "…"; long.Body() != want { + t.Fatalf("Body() has %d bytes, want %d", len(long.Body()), len(want)) } } diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 3e7c4c8..0d8c38d 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -74,15 +74,17 @@ var _ llm.Provider = (*Provider)(nil) // New validates cfg and returns a Provider. func New(cfg Config) (*Provider, error) { + // Base URL errors never echo the URL: a rejected one may carry a + // credential. u, err := url.Parse(cfg.BaseURL) if err != nil { return nil, errors.New("openai: base URL does not parse") } if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { - return nil, fmt.Errorf("openai: base URL %q must be an absolute http or https URL", u.Redacted()) + return nil, errors.New("openai: base URL must be an absolute http or https URL") } if u.User != nil || u.RawQuery != "" || u.Fragment != "" { - return nil, fmt.Errorf("openai: base URL %q must not carry credentials, a query or a fragment", u.Redacted()) + return nil, errors.New("openai: base URL must not carry credentials, a query or a fragment") } switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: @@ -217,7 +219,7 @@ func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, } if len(cr.Error) > 0 && string(cr.Error) != "null" { // OpenRouter can report an upstream failure in a 2xx body. - return llm.Response{}, fmt.Errorf("openai: generate: %w", llm.NewProviderError(status, raw, p.secrets())) + return llm.Response{}, fmt.Errorf("openai: generate: %w", llm.NewProviderError(status, raw, p.apiKey)) } if len(cr.Choices) == 0 { return llm.Response{}, errors.New("openai: generate: response has no choices") @@ -319,8 +321,6 @@ func (p *Provider) Verify(ctx context.Context) error { return err } -func (p *Provider) secrets() []string { return []string{p.apiKey} } - // do sends one request and returns the body and status of a 2xx response. // Any other status becomes the matching llm typed error. func (p *Provider) do(ctx context.Context, method, path string, payload []byte) ([]byte, int, error) { @@ -376,7 +376,7 @@ func (p *Provider) statusError(resp *http.Response, body []byte) error { case http.StatusRequestTimeout: return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) default: - return llm.NewProviderError(resp.StatusCode, body, p.secrets()) + return llm.NewProviderError(resp.StatusCode, body, p.apiKey) } } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 2f13e5c..a7bd6c6 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -498,6 +498,64 @@ func TestTransportErrorHasNoSecret(t *testing.T) { assertNoSecret(t, err) } +// TestKeyedErrorsNeverLeakKey drives every error path of the keyed OpenAI +// and OpenRouter shapes with vendor bodies that echo the key. +func TestKeyedErrorsNeverLeakKey(t *testing.T) { + shapes := map[string]struct { + path string + headers map[string]string + }{ + "OpenAI": {path: "/v1"}, + "OpenRouter": {path: "/api/v1", headers: map[string]string{"HTTP-Referer": "https://mealquest.example", "X-Title": "MealQuest"}}, + } + fixtures := []string{"openai_error_invalid_key.json", "openai_error_server.json", "openrouter_error_rate_limited.json"} + statuses := []int{400, 401, 403, 404, 408, 429, 500, 502, 503} + for name, shape := range shapes { + for _, fixture := range fixtures { + for _, status := range statuses { + vs := newVendorServer(t, reply{status: status, fixture: fixture}) + p := newProvider(t, vs.URL+shape.path, JSONSchema, shape.headers) + _, genErr := p.Generate(context.Background(), schemaRequest()) + _, listErr := p.ListModels(context.Background()) + verifyErr := p.Verify(context.Background()) + for _, err := range []error{genErr, listErr, verifyErr} { + if err == nil { + t.Fatalf("%s %s HTTP %d: nil error", name, fixture, status) + } + assertNoSecret(t, err) + var pe *llm.ProviderError + if errors.As(err, &pe) { + assertNoSecret(t, errors.New(pe.Body())) + } + } + } + } + } +} + +func TestKeylessOllamaErrorKeepsExcerpt(t *testing.T) { + vs := newVendorServer(t, reply{status: 404, fixture: "ollama_error_not_found.json"}) + p, err := New(Config{BaseURL: vs.URL + "/v1", Model: "llama9", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + _, err = p.Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.As(err, &pe) || pe.Status() != 404 { + t.Fatalf("err = %v, want a 404 ProviderError", err) + } + if !strings.Contains(pe.Body(), `model \"llama9\" not found`) || len(pe.Body()) > llm.MaxBodyExcerpt { + t.Fatalf("Body() = %q, want the bounded not-found explanation", pe.Body()) + } + if !strings.Contains(err.Error(), pe.Body()) { + t.Fatalf("Error() = %q, want it to carry the excerpt", err) + } + assertNoSecret(t, err) + if got := vs.only(t).header.Get("Authorization"); got != "" { + t.Fatalf("keyless request sent Authorization %q", got) + } +} + func TestGenerateRejectsInvalidRequests(t *testing.T) { vs := newVendorServer(t, reply{status: 200, fixture: "ollama_chat_text.json"}) p := newProvider(t, vs.URL, JSONSchema, nil) @@ -524,7 +582,11 @@ func TestNewRejectsBadConfig(t *testing.T) { "relative base": {BaseURL: "api.openai.com/v1", StructuredOutput: JSONSchema}, "ftp base": {BaseURL: "ftp://example.com", StructuredOutput: JSONSchema}, "credentials in URL": {BaseURL: "https://user:" + testKey + "@example.com/v1", StructuredOutput: JSONSchema}, - "query in URL": {BaseURL: "https://example.com/v1?key=x", StructuredOutput: JSONSchema}, + "key in query": {BaseURL: "https://example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, + "key in fragment": {BaseURL: "https://example.com/v1#" + testKey, StructuredOutput: JSONSchema}, + "key, relative base": {BaseURL: "example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, + "key, ftp base": {BaseURL: "ftp://example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, + "key, unparseable": {BaseURL: "https://example.com/%zz?key=" + testKey, StructuredOutput: JSONSchema}, "unknown mode": {BaseURL: "https://example.com/v1", StructuredOutput: "json"}, "no mode": {BaseURL: "https://example.com/v1"}, "authorization hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"authorization": "Bearer x"}},