diff --git a/internal/ai/llm/llm.go b/internal/ai/llm/llm.go index ce2aaad..23e7b9a 100644 --- a/internal/ai/llm/llm.go +++ b/internal/ai/llm/llm.go @@ -67,14 +67,15 @@ type Request struct { } // Validate reports whether r is well formed. Adapters call it before -// contacting the vendor. +// contacting the vendor. Its errors never quote the request's text, such as +// a role or schema name: any of it may equal the API key. func (r Request) Validate() error { if len(r.Messages) == 0 { return errors.New("llm: request has no messages") } for i, m := range r.Messages { if m.Role != RoleUser && m.Role != RoleAssistant { - return fmt.Errorf("llm: message %d has unknown role %q", i, m.Role) + return fmt.Errorf("llm: message %d has an unknown role", i) } } if r.Temperature != nil && *r.Temperature < 0 { @@ -92,7 +93,7 @@ func (r Request) Validate() error { } var obj map[string]json.RawMessage if err := json.Unmarshal(r.Schema.Schema, &obj); err != nil || obj == nil { - return fmt.Errorf("llm: schema %q is not a JSON object", r.Schema.Name) + return errors.New("llm: schema is not a JSON object") } } return nil diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 44828d7..cc4c13f 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -3,7 +3,9 @@ // POST {base}/chat/completions and GET {base}/models over plain net/http. // // The adapter never logs, and no error it returns contains the API key or -// the Authorization header value. +// the Authorization header value: errors do not quote caller- or +// vendor-supplied text, and every exported method passes its error through +// redact as a backstop. package openai import ( @@ -76,6 +78,14 @@ var _ llm.Provider = (*Provider)(nil) // New validates cfg and returns a Provider. func New(cfg Config) (*Provider, error) { + p, err := newFromConfig(cfg) + if err != nil { + return nil, redact(err, cfg.APIKey) + } + return p, nil +} + +func newFromConfig(cfg Config) (*Provider, error) { // Base URL errors never echo the URL: a rejected one may carry a // credential. u, err := url.Parse(cfg.BaseURL) @@ -219,6 +229,11 @@ const schemaInstruction = "Reply with only a JSON object, and no other text, tha // Generate sends req to {base}/chat/completions. func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, error) { + resp, err := p.generate(ctx, req) + return resp, redact(err, p.apiKey) +} + +func (p *Provider) generate(ctx context.Context, req llm.Request) (llm.Response, error) { if err := req.Validate(); err != nil { return llm.Response{}, fmt.Errorf("openai: generate: %w", err) } @@ -298,7 +313,7 @@ func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, resp.JSON = json.RawMessage(text) } if resp.JSON == nil { - return resp, fmt.Errorf("openai: generate: reply (finish reason %q) holds no JSON object: %w", resp.FinishReason, llm.ErrBadOutput) + return resp, fmt.Errorf("openai: generate: reply holds no JSON object: %w", llm.ErrBadOutput) } return resp, nil } @@ -343,6 +358,11 @@ type modelsResponse struct { // ListModels returns the models {base}/models lists. func (p *Provider) ListModels(ctx context.Context) ([]llm.Model, error) { + models, err := p.listModels(ctx) + return models, redact(err, p.apiKey) +} + +func (p *Provider) listModels(ctx context.Context) ([]llm.Model, error) { raw, _, err := p.do(ctx, http.MethodGet, "/models", nil) if err != nil { return nil, fmt.Errorf("openai: list models: %w", err) @@ -370,8 +390,8 @@ func (p *Provider) ListModels(ctx context.Context) ([]llm.Model, error) { // Verify checks the credentials with an authenticated ListModels call. func (p *Provider) Verify(ctx context.Context) error { - _, err := p.ListModels(ctx) - return err + _, err := p.listModels(ctx) + return redact(err, p.apiKey) } // do sends one request and returns the body and status of a 2xx response. @@ -493,10 +513,44 @@ func (p *Provider) transportError(err error) error { return &transportFailure{detail: redactKey(err.Error(), p.apiKey), kind: kind} } -// redactKey replaces every occurrence of a non-empty key in s. +// 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 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. +type redactedError struct { + text string + err error +} + +func (e *redactedError) Error() string { return e.text } + +func (e *redactedError) Unwrap() error { return e.err } + +// 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. +func redact(err error, key string) error { + if err == nil || key == "" { + return err + } + text := err.Error() + if !strings.Contains(text, key) { + return err + } + text = redactKey(text, key) + if strings.Contains(text, key) { + text = withheldText + } + return &redactedError{text: text, err: err} +} diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 745da45..cc45f32 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -785,3 +786,101 @@ func TestRetryAfter(t *testing.T) { } } } + +// headerNameKey is an API key that is also a word callers and vendors put in +// text: an error that quotes such text prints the key. +const headerNameKey = "Authorization" + +// assertNoKey fails if err's text holds key or "Bearer "+key in any case. +func assertNoKey(t *testing.T, err error, key string) { + t.Helper() + for _, secret := range []string{key, "Bearer " + key} { + if strings.Contains(err.Error(), secret) || strings.Contains(strings.ToLower(err.Error()), strings.ToLower(secret)) { + t.Fatalf("error leaks %q: %v", secret, err) + } + } +} + +// keyServer replies to every request with status and body. +func keyServer(t *testing.T, 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") + w.WriteHeader(status) + io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: headerNameKey, Model: "test-model", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + return p +} + +func TestGenerateInvalidRoleNeverLeaksKey(t *testing.T) { + p := keyServer(t, 200, `{}`) + _, err := p.Generate(context.Background(), llm.Request{Messages: []llm.Message{{Role: llm.Role(headerNameKey), Content: "hi"}}}) + if err == nil { + t.Fatal("Generate accepted an unknown role") + } + assertNoKey(t, err, headerNameKey) +} + +func TestGenerateInvalidSchemaNeverLeaksKey(t *testing.T) { + p := keyServer(t, 200, `{}`) + req := schemaRequest() + req.Schema = &llm.JSONSchema{Name: headerNameKey, Schema: json.RawMessage(`[]`)} + _, err := p.Generate(context.Background(), req) + if err == nil { + t.Fatal("Generate accepted a schema that is not a JSON object") + } + assertNoKey(t, err, headerNameKey) +} + +func TestGenerateBadOutputNeverLeaksKey(t *testing.T) { + p := keyServer(t, 200, `{"model":"m","choices":[{"message":{"content":"no JSON here"},"finish_reason":"`+headerNameKey+`"}]}`) + resp, err := p.Generate(context.Background(), schemaRequest()) + if !errors.Is(err, llm.ErrBadOutput) { + t.Fatalf("err = %v, want ErrBadOutput", err) + } + assertNoKey(t, err, headerNameKey) + if resp.FinishReason != headerNameKey { + t.Fatalf("FinishReason = %q, want it kept on the response", resp.FinishReason) + } +} + +func TestProviderErrorBodyNeverLeaksKey(t *testing.T) { + p := keyServer(t, 500, `{"error":{"message":"rejected `+headerNameKey+`: Bearer `+headerNameKey+`"}}`) + _, genErr := p.Generate(context.Background(), schemaRequest()) + _, listErr := p.ListModels(context.Background()) + verifyErr := p.Verify(context.Background()) + for _, err := range []error{genErr, listErr, verifyErr} { + 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) + } + assertNoKey(t, err, headerNameKey) + } +} + +// TestRedactKeepsChainAndHidesKey checks the backstop every exported method +// applies: the key and its Bearer value leave the text, the chain stays. +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 { + t.Fatalf("redact broke the chain of %v", err) + } + if redact(inner, "") != inner { + t.Fatal("redact changed an error for the empty key") + } + clean := errors.New("openai: generate: no model configured") + if redact(clean, testKey) != clean { + t.Fatal("redact wrapped an error that holds no key") + } + // A key inside the redaction marker would survive replacement. + if got := redact(errors.New("x E y"), "E").Error(); strings.Contains(got, "E") { + t.Fatalf("redact kept the key: %q", got) + } +}