From 3ddc18fa0de5825f7fe8a8ed13dade4c1edf6308 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:45:44 -0400 Subject: [PATCH] Redact the API key from all config errors and reject empty URL delimiters An invalid extra-header name is no longer echoed, and every other New error that quotes caller-supplied text (structured output mode, header names) has the API key redacted first via a shared redactKey helper, also used for transport errors. A base URL containing "?" or "#" is rejected even when the query or fragment is empty, so endpoint paths can no longer land in either. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 27 +++++++++++++++++--------- internal/ai/llm/openai/openai_test.go | 28 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 5a1b4e3..f897689 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -85,26 +85,31 @@ func New(cfg Config) (*Provider, error) { if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { return nil, errors.New("openai: base URL must be an absolute http or https URL") } - if u.User != nil || u.RawQuery != "" || u.Fragment != "" { + // A trailing "?" or "#" parses to an empty query or fragment, yet the + // endpoint path appended to it would land in the query or fragment. + if u.User != nil || strings.ContainsAny(cfg.BaseURL, "?#") { return nil, errors.New("openai: base URL must not carry credentials, a query or a fragment") } + // Every other error that names caller-supplied text has the API key + // redacted from that text before it is quoted. switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: default: - return nil, fmt.Errorf("openai: unknown structured output mode %q", cfg.StructuredOutput) + return nil, fmt.Errorf("openai: unknown structured output mode %q", redactKey(string(cfg.StructuredOutput), cfg.APIKey)) } headers := make(http.Header, len(cfg.Headers)+1) for name, value := range cfg.Headers { - // Values are never echoed: one may carry a credential. + // Invalid names and all values are never echoed: either may carry a + // credential. if !validHeaderName(name) { - return nil, fmt.Errorf("openai: extra header name %q is not a valid HTTP token", 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) } if !validHeaderValue(value) { - return nil, fmt.Errorf("openai: extra header %q has a control character in its value", canonical) + return nil, fmt.Errorf("openai: extra header %q has a control character in its value", redactKey(canonical, cfg.APIKey)) } headers.Set(canonical, value) } @@ -486,9 +491,13 @@ func (p *Provider) transportError(err error) error { if errors.As(err, &ue) { err = ue.Err } - detail := err.Error() - if p.apiKey != "" { - detail = strings.ReplaceAll(detail, p.apiKey, "[REDACTED]") + return &transportFailure{detail: redactKey(err.Error(), p.apiKey), kind: kind} +} + +// redactKey replaces every occurrence of a non-empty key in s. +func redactKey(s, key string) string { + if key == "" { + return s } - return &transportFailure{detail: detail, kind: kind} + return strings.ReplaceAll(s, key, "[REDACTED]") } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 30d2dfd..dc55844 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -627,6 +627,31 @@ func TestNewRejectsBadHeaderWithoutEchoingValue(t *testing.T) { assertNoSecret(t, err) } +// TestNewConfigErrorsNeverLeakKey covers every New error that names +// caller-supplied text, each with the API key embedded in that text. +func TestNewConfigErrorsNeverLeakKey(t *testing.T) { + cases := map[string]Config{ + "key in invalid header name": {Headers: map[string]string{"Bearer " + testKey: "a"}}, + "key as invalid header name": {Headers: map[string]string{testKey + ":": "a"}}, + "key in name, bad value": {Headers: map[string]string{"X-" + testKey: "a\nb"}}, + "key in structured output": {StructuredOutput: StructuredOutput("Bearer " + testKey)}, + "key with quote in mode": {StructuredOutput: StructuredOutput(testKey + "\"\x00")}, + } + for name, cfg := range cases { + cfg.BaseURL = "https://example.com/v1" + cfg.APIKey = testKey + if cfg.StructuredOutput == "" { + cfg.StructuredOutput = JSONSchema + } + _, err := New(cfg) + if err == nil { + t.Errorf("%s: New() = nil error", name) + continue + } + assertNoSecret(t, err) + } +} + func TestAcceptHeader(t *testing.T) { for name, tc := range map[string]struct { headers map[string]string @@ -664,6 +689,9 @@ func TestNewRejectsBadConfig(t *testing.T) { "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}, + "empty query": {BaseURL: "https://example.com/v1?", StructuredOutput: JSONSchema}, + "empty fragment": {BaseURL: "https://example.com/v1#", StructuredOutput: JSONSchema}, + "empty both": {BaseURL: "https://example.com/v1?#", 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"}},