diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index cdd4265..44828d7 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -90,25 +90,22 @@ func New(cfg Config) (*Provider, error) { if u.User != nil || strings.ContainsAny(cfg.BaseURL, "?#") { return nil, errors.New("openai: base URL must not carry credentials, a query or a fragment") } - // An error that names caller-supplied text has the API key redacted from - // it before it is quoted. + // No error from New quotes caller-supplied text or a name derived from + // it: any such text may equal or contain the API key, and redaction + // cannot catch a fragment or a case-changed copy of it. switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: default: - return nil, fmt.Errorf("openai: unknown structured output mode %q", redactKey(string(cfg.StructuredOutput), cfg.APIKey)) + return nil, errors.New("openai: unknown structured output mode") } headers := make(http.Header, len(cfg.Headers)+1) for name, value := range cfg.Headers { - // Caller-supplied names and values are never echoed: either may carry - // a credential, and canonicalising a name changes its case, which - // defeats redaction. Only a managed name, which is a fixed known - // string, is named. if !validHeaderName(name) { return nil, errors.New("openai: an extra header name is not a valid HTTP token") } canonical := textproto.CanonicalMIMEHeaderKey(name) if managedHeaders[canonical] { - return nil, fmt.Errorf("openai: extra header %q is set by the adapter", canonical) + return nil, errors.New("openai: an extra header is set by the adapter") } if !validHeaderValue(value) { return nil, errors.New("openai: an extra header value has a control character") diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 6feff02..745da45 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -652,6 +652,38 @@ func TestNewConfigErrorsNeverLeakKey(t *testing.T) { } } +// TestNewManagedHeaderErrorNeverLeaksKey uses an API key that is itself a +// managed header name, so naming the rejected header would print the key. +func TestNewManagedHeaderErrorNeverLeaksKey(t *testing.T) { + for _, key := range []string{"Authorization", "Content-Type", "Host"} { + _, err := New(Config{ + BaseURL: "https://example.com/v1", + APIKey: key, + StructuredOutput: JSONSchema, + Headers: map[string]string{key: "x"}, + }) + if err == nil { + t.Errorf("key %q: New() = nil error", key) + continue + } + for _, secret := range []string{key, "Bearer " + key} { + if strings.Contains(strings.ToLower(err.Error()), strings.ToLower(secret)) { + t.Errorf("key %q: error leaks %q: %v", key, secret, err) + } + } + } +} + +// TestNewModeErrorNeverLeaksKeyFragment uses a structured output mode that is +// a fragment of the API key, which redacting the whole key would not catch. +func TestNewModeErrorNeverLeaksKeyFragment(t *testing.T) { + _, err := New(Config{BaseURL: "https://example.com/v1", APIKey: testKey, StructuredOutput: StructuredOutput(testKey[:20])}) + if err == nil { + t.Fatal("New() = nil error") + } + assertNoSecret(t, err) +} + func TestAcceptHeader(t *testing.T) { for name, tc := range map[string]struct { headers map[string]string