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 <noreply@anthropic.com>

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

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 {

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)
+		}
 	}
 }
 

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))