diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 4f21be7..70c94e9 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -53,6 +53,7 @@ type Config struct { BaseURL string // APIKey is sent as "Authorization: Bearer ". Empty sends no // Authorization header, for endpoints such as Ollama that need none. + // It must not have a control character other than tab. APIKey string // Model is the model Generate requests. ListModels and Verify do not // need it. @@ -102,6 +103,9 @@ func newFromConfig(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") } + if !validHeaderValue(cfg.APIKey) { + return nil, errors.New("openai: API key has a control character") + } // 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. diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index c953fe1..12f2c09 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -628,6 +628,20 @@ func TestNewRejectsBadHeaderWithoutEchoingValue(t *testing.T) { assertNoSecret(t, err) } +func TestNewRejectsKeyWithControlCharacter(t *testing.T) { + for _, key := range []string{testKey + "\n", testKey + "\r\nX-Injected: 1", "\x00" + testKey, testKey + "\x7f"} { + _, err := New(Config{BaseURL: "https://example.com/v1", APIKey: key, StructuredOutput: JSONSchema}) + if err == nil { + t.Errorf("New(%q) = nil error", key) + continue + } + assertNoSecret(t, err) + if strings.Contains(err.Error()+fmt.Sprintf("%#v", err), key) { + t.Errorf("error leaks the key: %v", 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) { diff --git a/internal/ai/llm/presets/presets.go b/internal/ai/llm/presets/presets.go index a05c675..d33d613 100644 --- a/internal/ai/llm/presets/presets.go +++ b/internal/ai/llm/presets/presets.go @@ -164,7 +164,7 @@ var all = map[string]Preset{ // FromSettings returns the Provider s describes, using key as its API key. // It fails with a configuration error from package llm — ErrUnknownProvider, -// ErrMissingKey, ErrUnexpectedKey, ErrMissingBaseURL, ErrMissingModel, +// ErrMissingKey, ErrUnexpectedKey, ErrInvalidKey, ErrMissingBaseURL, ErrMissingModel, // ErrMissingPublicURL or ErrInvalidSetting — before contacting any vendor. // // The Provider sends s.Temperature with every request whose Temperature is @@ -230,6 +230,8 @@ func check(p Preset, s llm.Settings, key string) error { return llm.ErrMissingKey case p.Key == KeyNone && key != "": return llm.ErrUnexpectedKey + case !validHeaderValue(key): + return llm.ErrInvalidKey } switch { case p.BaseURL == "" && s.BaseURL == "": @@ -252,6 +254,17 @@ func check(p Preset, s llm.Settings, key string) error { return nil } +// validHeaderValue reports whether value has no control character other +// than tab, so it can be sent as an HTTP header value. +func validHeaderValue(value string) bool { + for i := 0; i < len(value); i++ { + if c := value[i]; (c < 0x20 && c != '\t') || c == 0x7f { + return false + } + } + return true +} + // configured applies the household temperature and the preset's // temperature quirk to every Generate. type configured struct { diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go index 78c6b2c..47f98c5 100644 --- a/internal/ai/llm/presets/presets_test.go +++ b/internal/ai/llm/presets/presets_test.go @@ -337,7 +337,7 @@ func TestPresetTable(t *testing.T) { func TestConfigErrors(t *testing.T) { sentinels := []error{ - llm.ErrUnknownProvider, llm.ErrMissingKey, llm.ErrUnexpectedKey, llm.ErrMissingBaseURL, + llm.ErrUnknownProvider, llm.ErrMissingKey, llm.ErrUnexpectedKey, llm.ErrInvalidKey, llm.ErrMissingBaseURL, llm.ErrMissingModel, llm.ErrMissingPublicURL, llm.ErrInvalidSetting, } cases := []struct { @@ -351,6 +351,10 @@ func TestConfigErrors(t *testing.T) { {"openai no key", llm.Settings{Provider: "openai", Model: "m"}, "", llm.ErrMissingKey}, {"openrouter no key", llm.Settings{Provider: "openrouter", Model: "m", PublicURL: "https://x.example"}, "", llm.ErrMissingKey}, {"ollama with key", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, testKey, llm.ErrUnexpectedKey}, + {"anthropic key with newline", llm.Settings{Provider: "anthropic", Model: "m"}, testKey + "\n", llm.ErrInvalidKey}, + {"openai key with newline", llm.Settings{Provider: "openai", Model: "m"}, testKey + "\n", llm.ErrInvalidKey}, + {"openrouter key with CR LF", llm.Settings{Provider: "openrouter", Model: "m", PublicURL: "https://x.example"}, testKey + "\r\nX-Injected: 1", llm.ErrInvalidKey}, + {"openai_compatible key with NUL", llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1", Model: "m"}, "\x00" + testKey, llm.ErrInvalidKey}, {"ollama no base URL", llm.Settings{Provider: "ollama", Model: "m"}, "", llm.ErrMissingBaseURL}, {"openai_compatible no base URL", llm.Settings{Provider: "openai_compatible", Model: "m"}, "", llm.ErrMissingBaseURL}, {"anthropic no model", llm.Settings{Provider: "anthropic"}, testKey, llm.ErrMissingModel}, @@ -449,6 +453,8 @@ func TestConfigErrorsNeverLeakKey(t *testing.T) { {"key as temperature", llm.Settings{Provider: "openai", Model: "m", Temperature: -0.1}, "-0.1", llm.ErrInvalidSetting}, {"key in bad base URL", llm.Settings{Provider: "openai_compatible", BaseURL: "ftp://k3y", Model: "m"}, "k3y", llm.ErrInvalidSetting}, {"ollama with key", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, "key", llm.ErrUnexpectedKey}, + {"key with newline", llm.Settings{Provider: "openai", Model: "m"}, "sk-test\n", llm.ErrInvalidKey}, + {"key with newline as text", llm.Settings{Provider: "openai", Model: "m"}, "header\n", llm.ErrInvalidKey}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/internal/ai/llm/settings.go b/internal/ai/llm/settings.go index a9333a7..adde36c 100644 --- a/internal/ai/llm/settings.go +++ b/internal/ai/llm/settings.go @@ -50,6 +50,9 @@ var ( // ErrUnexpectedKey means an API key was given to a preset that sends // none. ErrUnexpectedKey = errors.New("llm: provider takes no API key") + // ErrInvalidKey means the API key is not a valid HTTP header value: + // it has a control character such as CR or LF. + ErrInvalidKey = errors.New("llm: API key is not a valid header value") // ErrMissingBaseURL means the preset requires a base URL and none was // given. ErrMissingBaseURL = errors.New("llm: base URL is required")