diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index db126bf..c6e0e82 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -166,7 +166,8 @@ automatic** (D-021). 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 + live from the provider, with the key redacted from every id and + display_name. Both work before a model is chosen (`presets.ForAccount`), answer 409 `not_configured` when the settings cannot build a provider, and 502 with the provider's typed error (`auth_failed`, `rate_limited`, `bad_output`, `provider_error`; 504 diff --git a/internal/ai/aisettings/aisettings.go b/internal/ai/aisettings/aisettings.go index 6227047..cfc7dcd 100644 --- a/internal/ai/aisettings/aisettings.go +++ b/internal/ai/aisettings/aisettings.go @@ -217,7 +217,8 @@ func (s *Service) Verify(ctx context.Context) (Settings, error) { }) } -// Models returns the provider's live model list. Its errors are Verify's. +// Models returns the provider's live model list with the API key removed +// from every model's ID and display name. Its errors are Verify's. func (s *Service) Models(ctx context.Context) ([]Model, error) { row, err := s.read(ctx, db.New(s.st.Reader)) if err != nil { @@ -233,7 +234,7 @@ func (s *Service) Models(ctx context.Context) ([]Model, error) { } out := make([]Model, len(models)) for i, m := range models { - out[i] = Model{ID: m.ID, DisplayName: m.DisplayName} + out[i] = Model{ID: llm.Redact(m.ID, key), DisplayName: llm.Redact(m.DisplayName, key)} } return out, nil } diff --git a/internal/ai/aisettings/aisettings_test.go b/internal/ai/aisettings/aisettings_test.go index ed80e9e..3bf9fe8 100644 --- a/internal/ai/aisettings/aisettings_test.go +++ b/internal/ai/aisettings/aisettings_test.go @@ -352,6 +352,11 @@ func TestVerifyRedactsKey(t *testing.T) { if _, err := s.Models(ctx); !errors.Is(err, ErrProviderFailed) || strings.Contains(err.Error(), testKey) { t.Errorf("Models = %v, want ErrProviderFailed without the key", err) } + f.QueueListModels(llm.ListModelsResult{Models: []llm.Model{{ID: "m-" + testKey, DisplayName: "Model " + testKey}}}) + models, err := s.Models(ctx) + if err != nil || len(models) != 1 || models[0] != (Model{ID: "m-[REDACTED]", DisplayName: "Model [REDACTED]"}) { + t.Errorf("Models = %+v, %v, want the key redacted", models, err) + } } // Settings changed while the provider verifies the key are not marked diff --git a/internal/server/aisettings.go b/internal/server/aisettings.go index 68d6985..88db4fd 100644 --- a/internal/server/aisettings.go +++ b/internal/server/aisettings.go @@ -21,8 +21,8 @@ type modelsResponse struct { } // No response from these handlers holds the API key: the settings carry -// only its last four characters, and provider errors are redacted by -// package aisettings. +// only its last four characters, and provider errors and model lists are +// redacted by package aisettings. func getAISettings(logger *slog.Logger, svc *aisettings.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -38,7 +38,7 @@ func getAISettings(logger *slog.Logger, svc *aisettings.Service) http.HandlerFun func patchAISettings(logger *slog.Logger, svc *aisettings.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - patch, err := decodePatch[aisettings.Patch](http.MaxBytesReader(w, r.Body, maxSettingsBody)) + patch, err := decodePatch[aisettings.Patch](http.MaxBytesReader(w, r.Body, maxAISettingsBody)) if err != nil { writeError(w, r, logger, http.StatusBadRequest, "invalid_request", err.Error()) return diff --git a/internal/server/aisettings_test.go b/internal/server/aisettings_test.go index a7feea7..8bbd281 100644 --- a/internal/server/aisettings_test.go +++ b/internal/server/aisettings_test.go @@ -9,6 +9,7 @@ import ( "sync/atomic" "testing" "testing/fstest" + "unicode/utf8" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household" ) @@ -38,7 +39,8 @@ func newAIRouter(t *testing.T) http.Handler { // newModelsServer is an OpenAI-compatible server whose GET /models accepts // only testAIKey. While echo is set it answers 500 with a body quoting the // Authorization header it received, so a response that passed the vendor -// body through unredacted would leak the key. +// body through unredacted would leak the key. Its model list holds a model +// whose ID and name quote the key. func newModelsServer(t *testing.T, echo *atomic.Bool) *httptest.Server { t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -53,7 +55,8 @@ func newModelsServer(t *testing.T, echo *atomic.Bool) *httptest.Server { w.WriteHeader(http.StatusUnauthorized) io.WriteString(w, `{"error":{"message":"invalid key `+auth+`"}}`) default: - io.WriteString(w, `{"data":[{"id":"gpt-test"},{"id":"gpt-test-mini","name":"GPT Test Mini"}]}`) + io.WriteString(w, `{"data":[{"id":"gpt-test"},{"id":"gpt-test-mini","name":"GPT Test Mini"},`+ + `{"id":"gpt-`+testAIKey+`","name":"Leaky `+testAIKey+`"}]}`) } })) t.Cleanup(srv.Close) @@ -117,6 +120,8 @@ func TestAISettingsAPI(t *testing.T) { `{"model":""}`, `{"model":null}`, `{"system_prompt":"` + strings.Repeat("é", 8001) + `"}`, + `{"system_prompt":"` + strings.Repeat("€", 8001) + `"}`, + `{"system_prompt":"` + strings.Repeat("a", maxAISettingsBody) + `"}`, `{"api_key":"` + testAIKey + `"}`, } { if rec, body := call(http.MethodPatch, aiPath, bad); rec.Code != http.StatusBadRequest { @@ -124,6 +129,15 @@ func TestAISettingsAPI(t *testing.T) { } } + // The longest prompt fits the body limit however its 8,000 characters + // are written: three-byte, four-byte, or as \u-escaped surrogate pairs. + for _, char := range []string{"€", "🍲", `\ud83c\udf72`} { + rec, body := call(http.MethodPatch, aiPath, `{"system_prompt":"`+strings.Repeat(char, 8000)+`"}`) + if prompt, _ := body["system_prompt"].(string); rec.Code != http.StatusOK || utf8.RuneCountInString(prompt) != 8000 { + t.Errorf("PATCH 8,000 × %s = %d, %d characters", char, rec.Code, utf8.RuneCountInString(prompt)) + } + } + rec, body = call(http.MethodPatch, aiPath, `{"provider":"openai_compatible","base_url":"`+vendor.URL+`/v1","model":"gpt-test","temperature":0.3,"system_prompt":"`+strings.Repeat("é", 8000)+`"}`) if rec.Code != http.StatusOK || body["provider"] != "openai_compatible" || body["base_url"] != vendor.URL+"/v1" || body["model"] != "gpt-test" || body["temperature"] != 0.3 { @@ -178,12 +192,15 @@ func TestAISettingsAPI(t *testing.T) { rec, body = call(http.MethodGet, aiModelsPath, "") models, _ := body["models"].([]any) - if rec.Code != http.StatusOK || len(models) != 2 { + if rec.Code != http.StatusOK || len(models) != 3 { t.Fatalf("GET models = %d %v", rec.Code, body) } if m := models[1].(map[string]any); m["id"] != "gpt-test-mini" || m["display_name"] != "GPT Test Mini" { t.Errorf("GET models[1] = %v", m) } + if m := models[2].(map[string]any); m["id"] != "gpt-[REDACTED]" || m["display_name"] != "Leaky [REDACTED]" { + t.Errorf("GET models[2] = %v, want the key redacted", m) + } rec, body = call(http.MethodPatch, aiPath, `{"system_prompt":"Be brief."}`) if rec.Code != http.StatusOK || body["system_prompt"] != "Be brief." || body["key_verified_at"] == nil { diff --git a/internal/server/settings.go b/internal/server/settings.go index 032dbed..489d092 100644 --- a/internal/server/settings.go +++ b/internal/server/settings.go @@ -9,14 +9,21 @@ import ( "log/slog" "net/http" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/aisettings" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household" ) -// maxSettingsBody bounds a PATCH /settings/household, /settings/ai or +// maxSettingsBody bounds a PATCH /settings/household or // /settings/recipe-search, or PUT /settings/ai/api-key, request body. const maxSettingsBody = 16 << 10 +// maxAISettingsBody bounds a PATCH /settings/ai request body. It fits the +// longest valid system prompt at 12 bytes of JSON per character (a +// \u-escaped surrogate pair) on top of maxSettingsBody for the other +// fields, so only aisettings.MaxSystemPromptChars limits the prompt. +const maxAISettingsBody = maxSettingsBody + 12*aisettings.MaxSystemPromptChars + // settingsPatch is the PATCH /settings/household body. Every field is // optional; unknown fields and null values are refused. type settingsPatch struct {