From 94632e09500e010a31f673e1fc0cf37f840e724e Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 08:52:01 -0400 Subject: [PATCH] Keep the stored AI key on the endpoint it was stored for PATCH /settings/ai refused nothing when the base URL or provider changed with a key stored, so a session could point base_url at another server and have POST /settings/ai/verify send it the key. Both changes are now refused while a key is stored: remove the key, change the endpoint, then store the new key. A preset without a fixed base URL takes a key only when its base URL passes secureurl.Check, so the key never crosses the network in plain text. Maestro task: 176b2a30-6e90-4339-b430-5b8967b2fd32 Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 8 ++- internal/ai/aisettings/aisettings.go | 36 +++++++--- internal/ai/aisettings/aisettings_test.go | 87 ++++++++++++++++++----- internal/server/aisettings_test.go | 16 +++-- 4 files changed, 112 insertions(+), 35 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 658f16b..db126bf 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -157,11 +157,13 @@ automatic** (D-021). `api_key_last4` and `key_verified_at`. PATCH requires a preset name as provider, temperature 0–1 in 0.1 steps, a non-empty model and a system_prompt of at most 8000 characters; a fixed-URL preset's base_url - must be empty or its own. Changing provider or base_url clears - `key_verified_at`. + must be empty or its own. Provider and base_url cannot change while a + key is stored (remove the key first), so a stored key is never sent to + an endpoint it was not stored for. `PUT /api/v1/settings/ai/api-key` takes `{api_key}` (write-only, at least 8 characters, sealed with the secret box; an empty string removes it) and - clears `key_verified_at`. `POST /api/v1/settings/ai/verify` calls + clears `key_verified_at`; a preset without a fixed base_url takes a key + only when its base_url passes `secureurl.Check`. `POST /api/v1/settings/ai/verify` calls `Provider.Verify` and records `key_verified_at`; `GET /api/v1/settings/ai/models` returns `{models: [{id, display_name}]}` live from the provider. Both work before a model is chosen diff --git a/internal/ai/aisettings/aisettings.go b/internal/ai/aisettings/aisettings.go index 3d9bc65..6227047 100644 --- a/internal/ai/aisettings/aisettings.go +++ b/internal/ai/aisettings/aisettings.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "math" + "net/url" "strings" "unicode/utf8" @@ -20,6 +21,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secretbox" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secureurl" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" ) @@ -134,9 +136,10 @@ func (s *Service) Get(ctx context.Context) (Settings, error) { return settingsFrom(row), nil } -// Update applies patch and returns the settings it leaves. Changing the -// provider or base URL clears key_verified_at. Every problem is reported -// in an *ValidationError and nothing changes. +// Update applies patch and returns the settings it leaves. The provider +// and base URL cannot change while an API key is stored, so the key never +// reaches an endpoint it was not stored for. Every problem is reported in +// a *ValidationError and nothing changes. func (s *Service) Update(ctx context.Context, patch Patch) (Settings, error) { return s.change(ctx, func(row db.AiSetting) (db.AiSetting, error) { return applyPatch(row, patch) @@ -144,7 +147,9 @@ func (s *Service) Update(ctx context.Context, patch Patch) (Settings, error) { } // SetAPIKey seals key and records its last four characters, clearing -// key_verified_at. An empty key removes the stored one. +// key_verified_at. An empty key removes the stored one. A preset without +// a fixed base URL takes a key only when its base URL is secure (see +// secureurl.Check). func (s *Service) SetAPIKey(ctx context.Context, key string) (Settings, error) { return s.change(ctx, func(row db.AiSetting) (db.AiSetting, error) { row.ApiKeyCt, row.ApiKeyLast4, row.KeyVerifiedAt = nil, sql.NullString{}, sql.NullString{} @@ -156,6 +161,8 @@ func (s *Service) SetAPIKey(ctx context.Context, key string) (Settings, error) { switch { case p.Key == presets.KeyNone: reason = row.Provider + " takes no API key" + case p.BaseURL == "" && !secureBaseURL(row.BaseUrl): + reason = "is sent only over https://, or http:// to localhost: set such a base_url first" case !validHeaderValue(key): reason = "has a character not allowed in a header" case len(key) < MinKeyLength: @@ -392,16 +399,21 @@ func applyPatch(row db.AiSetting, patch Patch) (db.AiSetting, error) { fail("base_url", err.Error()) } } - if p.Key == presets.KeyNone && len(row.ApiKeyCt) > 0 { - fail("provider", p.Name+" takes no API key: remove the stored key first") + } + // The stored key is only ever sent where it was stored for: moving it + // to another provider or server would let a changed base URL receive + // it. Remove the key, change the endpoint, then store the new key. + if len(problems) == 0 && len(row.ApiKeyCt) > 0 { + if row.Provider != before.Provider { + fail("provider", "cannot change while an API key is stored: remove the key first") + } + if row.BaseUrl != before.BaseUrl { + fail("base_url", "cannot change while an API key is stored: remove the key first") } } if len(problems) > 0 { return db.AiSetting{}, &ValidationError{Problems: problems} } - if row.Provider != before.Provider || row.BaseUrl != before.BaseUrl { - row.KeyVerifiedAt = sql.NullString{} - } return row, nil } @@ -425,6 +437,12 @@ func nullString(s sql.NullString) *string { return &s.String } +// secureBaseURL reports whether base is a URL a key may be sent to. +func secureBaseURL(base string) bool { + u, err := url.Parse(base) + return base != "" && err == nil && secureurl.Check(u) == nil +} + // validHeaderValue reports whether value has no control character other // than tab. func validHeaderValue(value string) bool { diff --git a/internal/ai/aisettings/aisettings_test.go b/internal/ai/aisettings/aisettings_test.go index e248185..ed80e9e 100644 --- a/internal/ai/aisettings/aisettings_test.go +++ b/internal/ai/aisettings/aisettings_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "reflect" + "slices" "strings" "sync" "testing" @@ -223,37 +224,89 @@ func TestUpdateAccepts(t *testing.T) { } } -// Changing the provider or base URL voids the key's verification; the -// model, temperature and system prompt do not. -func TestVerificationVoidedByProviderChange(t *testing.T) { +// While a key is stored the provider and base URL cannot change, so the +// key never reaches an endpoint it was not stored for; the model, +// temperature and system prompt can, keeping the verification. +func TestEndpointLockedWhileKeyStored(t *testing.T) { ctx := context.Background() s, _ := newService(t) f := &llm.Fake{} useFake(s, f) + if _, err := s.Update(ctx, Patch{Provider: ptr("openai_compatible"), BaseURL: ptr("https://llm.example/v1")}); err != nil { + t.Fatal(err) + } if _, err := s.SetAPIKey(ctx, testKey); err != nil { t.Fatal(err) } - verify := func() { - t.Helper() - f.QueueVerify(nil) - got, err := s.Verify(ctx) - if err != nil || got.KeyVerifiedAt == nil { - t.Fatalf("Verify = %+v, %v", got, err) - } + f.QueueVerify(nil) + if got, err := s.Verify(ctx); err != nil || got.KeyVerifiedAt == nil { + t.Fatalf("Verify = %+v, %v", got, err) } - verify() got, err := s.Update(ctx, Patch{Model: ptr("m"), Temperature: ptr(0.5), SystemPrompt: ptr("p")}) if err != nil || got.KeyVerifiedAt == nil { - t.Errorf("model change voided verification: %+v, %v", got, err) + t.Errorf("model change = %+v, %v; want verification kept", got, err) } - got, err = s.Update(ctx, Patch{Provider: ptr("openai")}) - if err != nil || got.KeyVerifiedAt != nil || got.APIKeyLast4 == nil { - t.Errorf("provider change = %+v, %v; want key kept, verification voided", got, err) + before, err := s.Get(ctx) + if err != nil { + t.Fatal(err) + } + for _, c := range []struct { + patch Patch + field string + }{ + {Patch{BaseURL: ptr("https://attacker.example/v1")}, "base_url"}, + {Patch{Provider: ptr("openai"), BaseURL: ptr("")}, "provider"}, + {Patch{Provider: ptr("ollama"), BaseURL: ptr("http://localhost:11434")}, "provider"}, + } { + var invalid *ValidationError + _, err := s.Update(ctx, c.patch) + if !errors.As(err, &invalid) || !slices.ContainsFunc(invalid.Problems, func(p Problem) bool { return p.Field == c.field }) { + t.Errorf("Update(%+v) with a key stored = %v, want a %s problem", c.patch, err, c.field) + } + } + if after, err := s.Get(ctx); err != nil || !reflect.DeepEqual(after, before) { + t.Errorf("refused endpoint change altered the settings: %+v, %v", after, err) + } + + // Removing the key frees the endpoint; a new key is unverified. + if _, err := s.SetAPIKey(ctx, ""); err != nil { + t.Fatal(err) + } + if _, err := s.Update(ctx, Patch{Provider: ptr("openai"), BaseURL: ptr("")}); err != nil { + t.Fatal(err) } - verify() got, err = s.SetAPIKey(ctx, "sk-another-key-9999") if err != nil || got.KeyVerifiedAt != nil || *got.APIKeyLast4 != "9999" { - t.Errorf("new key = %+v, %v; want verification voided", got, err) + t.Errorf("new key = %+v, %v; want it unverified", got, err) + } +} + +// A key is never stored for a base URL it would cross the network to in +// plain text. +func TestSetAPIKeyNeedsSecureBaseURL(t *testing.T) { + ctx := context.Background() + for _, c := range []struct { + base string + ok bool + }{ + {"", false}, + {"http://llm.lan:8080/v1", false}, + {"http://127.0.0.1:8080/v1", true}, + {"https://llm.example/v1", true}, + } { + s, _ := newService(t) + patch := Patch{Provider: ptr("openai_compatible")} + if c.base != "" { + patch.BaseURL = ptr(c.base) + } + if _, err := s.Update(ctx, patch); err != nil { + t.Fatal(err) + } + _, err := s.SetAPIKey(ctx, testKey) + var invalid *ValidationError + if c.ok != (err == nil) || (!c.ok && !errors.As(err, &invalid)) { + t.Errorf("SetAPIKey with base URL %q = %v, want ok %v", c.base, err, c.ok) + } } } diff --git a/internal/server/aisettings_test.go b/internal/server/aisettings_test.go index 31394b5..a7feea7 100644 --- a/internal/server/aisettings_test.go +++ b/internal/server/aisettings_test.go @@ -194,15 +194,19 @@ func TestAISettingsAPI(t *testing.T) { t.Errorf("POST reset-prompt = %d %v", rec.Code, body) } - // Pointing the key at another server voids its verification. - rec, body = call(http.MethodPatch, aiPath, `{"base_url":"https://llm.example/v1"}`) - if rec.Code != http.StatusOK || body["key_verified_at"] != nil || body["api_key_last4"] != "7d21" { - t.Errorf("PATCH base_url = %d %v", rec.Code, body) + // The stored key is never pointed at another server or provider. + for _, patch := range []string{`{"base_url":"https://attacker.example/v1"}`, `{"provider":"openai","base_url":""}`} { + if rec, body := call(http.MethodPatch, aiPath, patch); rec.Code != http.StatusBadRequest || errorCode(body) != "invalid_settings" { + t.Errorf("PATCH %s with a key stored = %d %v, want 400 invalid_settings", patch, rec.Code, body) + } } - - if rec, body := call(http.MethodPut, aiKeyPath, `{"api_key":""}`); rec.Code != http.StatusOK || body["api_key_last4"] != nil { + if rec, body := call(http.MethodPut, aiKeyPath, `{"api_key":""}`); rec.Code != http.StatusOK || body["api_key_last4"] != nil || body["key_verified_at"] != nil { t.Errorf("PUT empty key = %d %v", rec.Code, body) } + rec, body = call(http.MethodPatch, aiPath, `{"base_url":"https://llm.example/v1"}`) + if rec.Code != http.StatusOK || body["base_url"] != "https://llm.example/v1" { + t.Errorf("PATCH base_url without a key = %d %v", rec.Code, body) + } if len(bodies) < 30 { t.Fatalf("scanned only %d response bodies", len(bodies))