diff --git a/internal/gateway/api.go b/internal/gateway/api.go index 78c3e7c..9ff93dc 100644 --- a/internal/gateway/api.go +++ b/internal/gateway/api.go @@ -3,6 +3,7 @@ package gateway import ( "encoding/csv" "encoding/json" + "fmt" "io" "log" "net/http" @@ -83,6 +84,121 @@ type sourcePayload struct { // It exists because "add one model" is the most common scripted edit and a // full Models list cannot be written without reading the source first. ModelIDs []string `json:"model_ids,omitempty"` + + // The four fields below are POINTERS, and that is the whole point. + // + // POST/PUT is an upsert that rewrites the whole source, so any field the + // payload cannot express is silently reset to its zero value. That already + // destroyed api_key once (fixed with resolveAPIKey) and would equally + // destroy proxy_url, api_key_env, timeout and queue_timeout. + // + // A plain string/duration cannot distinguish "the caller did not mention + // this field" from "the caller asked for the empty value", and only the + // former may inherit. So: + // + // nil → field absent from the request: keep the current value + // &"x" → present: store exactly "x" (including "" to clear it) + // + // api_key deliberately keeps its older "empty means inherit" rule rather + // than gaining a pointer: that rule is already published (v1.7.6) and + // scripts depend on it. Changing it now would let a script that echoes an + // empty api_key erase a live credential, which is the exact failure this + // whole area exists to prevent. + ProxyURL *string `json:"proxy_url,omitempty"` + APIKeyEnv *string `json:"api_key_env,omitempty"` + Timeout *string `json:"timeout,omitempty"` // duration string, e.g. "300s" + QueueTimeout *string `json:"queue_timeout,omitempty"` // duration string, e.g. "90s" +} + +// optionalSourceFields carries the Source fields a partial payload must not +// reset, each as a pointer so "absent" and "explicitly empty" stay distinct. +// +// nil → absent from the request: keep the current value +// &"" → present: clear it +// &"x" → present: store "x" +// +// The alternative — a plain value with "empty means inherit" — cannot express +// "clear this field", so emptying the proxy input in the UI would silently keep +// the old proxy. It is exactly why api_key is NOT modelled this way: for a +// credential, failing to keep the old value is worse than failing to clear it, +// and that rule is already published (v1.7.6). Two fields with opposite failure +// modes therefore get opposite rules, and both are spelled out here. +type optionalSourceFields struct { + ProxyURL *string + APIKeyEnv *string + Timeout *string + QueueTimeout *string +} + +// resolve overlays the four optional fields onto payload. +// +// A nil pointer means the request did not mention the field, so it keeps the +// value already stored in cur (nil cur = a source being created, where the +// payload's own zero value is correct). A non-nil pointer always wins, including +// when it points at the empty string, which is how a UI form clears a field. +// +// Only these four fields are overlaid. Everything else in payload is taken as +// sent: an upsert that inherited the whole record would make it impossible to +// change anything. +func (o optionalSourceFields) resolve(payload config.Source, cur *config.Source) (config.Source, error) { + // cur is dereferenced exactly once here so the create path (nil) cannot + // panic on the per-field lookups below. + var storedProxyURL, storedKeyEnv string + var storedTimeout, storedQueueTimeout time.Duration + if cur != nil { + storedProxyURL, storedKeyEnv = cur.ProxyURL, cur.APIKeyEnv + storedTimeout, storedQueueTimeout = cur.Timeout, cur.QueueTimeout + } + pick := func(p *string, stored string) string { + if p != nil { + return *p + } + return stored + } + payload.ProxyURL = pick(o.ProxyURL, storedProxyURL) + payload.APIKeyEnv = pick(o.APIKeyEnv, storedKeyEnv) + if err := overlayDuration(&payload.Timeout, o.Timeout, "timeout", storedTimeout); err != nil { + return payload, err + } + if err := overlayDuration(&payload.QueueTimeout, o.QueueTimeout, "queue_timeout", storedQueueTimeout); err != nil { + return payload, err + } + return payload, nil +} + +// overlayDuration applies the absent/present rule to one duration field: a nil +// pointer keeps stored, a set pointer replaces it (with "" / "0s" clearing). +func overlayDuration(dst *time.Duration, p *string, field string, stored time.Duration) error { + if p == nil { + *dst = stored + return nil + } + d, err := parseOptionalDuration(*p, field) + if err != nil { + return err + } + *dst = d + return nil +} + +// parseOptionalDuration parses a duration written as a Go duration string. +// An empty string clears the field back to the config default (0), which is +// what a UI form submitting an empty timeout box should mean. Parsing happens +// only for fields the request actually mentions, so a typo can never surface +// as a silent reset of something else. +func parseOptionalDuration(v, field string) (time.Duration, error) { + v = strings.TrimSpace(v) + if v == "" { + return 0, nil + } + d, err := time.ParseDuration(v) + if err != nil { + return 0, fmt.Errorf("%s %q is not a duration (use e.g. 120s, 5m, 1h)", field, v) + } + if d < 0 { + return 0, fmt.Errorf("%s must be >= 0 (got %s)", field, v) + } + return d, nil } // keepExistingAPIKey is the mask a client sends when it means "keep the @@ -186,6 +302,21 @@ func (g *Gateway) handleSourcesAPI(w http.ResponseWriter, r *http.Request) { MaxConcurrent: p.MaxConcurrent, RPM: p.RPM, } + // The four upsert-fragile fields are applied last: a nil pointer means + // "not in the request", so they are inherited from the stored source + // instead of being reset to the payload's zero value. This runs whether + // or not the request mentioned them, because resolve() itself decides + // per field — passing them unconditionally keeps the rule in one place. + src, err := optionalSourceFields{ + ProxyURL: p.ProxyURL, + APIKeyEnv: p.APIKeyEnv, + Timeout: p.Timeout, + QueueTimeout: p.QueueTimeout, + }.resolve(src, g.sourceByName(p.Name)) + if err != nil { + writeError(w, http.StatusBadRequest, "source_error", err.Error()) + return + } // model_ids is additive: "add these models" is the common scripted edit // and it must not require reading (and echoing) the whole list back. // A request that omits models entirely is therefore a pure add, not a diff --git a/internal/gateway/apiv1.go b/internal/gateway/apiv1.go index 242e7ad..b49956d 100644 --- a/internal/gateway/apiv1.go +++ b/internal/gateway/apiv1.go @@ -78,7 +78,9 @@ func (g *Gateway) apiV1Routes(w http.ResponseWriter, r *http.Request) { "admin role required to reveal credentials") return } - writeJSON(w, http.StatusOK, map[string]interface{}{"source": s}) + // The edit form round-trips this response, so it needs the credential in the + // clear AND the durations — see maskSourceFields. + writeJSON(w, http.StatusOK, map[string]interface{}{"source": maskSourceFields(s, true, true)}) return } writeJSON(w, http.StatusOK, map[string]interface{}{"source": maskSource(s)}) @@ -176,7 +178,11 @@ func (g *Gateway) apiV1Index(w http.ResponseWriter, r *http.Request) { {Method: "GET", Path: "/api/keys", Auth: "admin", Summary: "gateway keys"}, {Method: "POST", Path: "/api/keys", Auth: "admin", Summary: "create a gateway key", WriteEffect: "writes config.yaml"}, - {Method: "DELETE", Path: "/api/keys/{name}", Auth: "admin", Summary: "delete a gateway key", + {Method: "PUT", Path: "/api/keys/{key}", Auth: "admin", + Summary: "update a gateway key (name, role, model scopes and their per-model quotas)", + WriteEffect: "writes config.yaml"}, + {Method: "DELETE", Path: "/api/keys/{key}", Auth: "admin", + Summary: "delete a gateway key — the path segment is the KEY itself, not its name", WriteEffect: "writes config.yaml"}, {Method: "GET", Path: "/api/status", Auth: "any", Summary: "per-source health detail"}, @@ -194,6 +200,14 @@ func (g *Gateway) apiV1Index(w http.ResponseWriter, r *http.Request) { "partial_update": "api_key may be omitted or sent as the literal \"__KEEP__\" to inherit the " + "current credential; model_ids adds models to the existing list instead of replacing it, " + "so a one-field edit never needs to read the source first", + "optional_fields": "proxy_url, api_key_env, timeout and queue_timeout use presence semantics: " + + "OMITTED from the request keeps the stored value, present (even as \"\") overwrites it. " + + "The upsert rewrites the whole source, so without this a one-field edit would silently reset " + + "them — and api_key_env in particular is only visible as a credential failure on the NEXT " + + "upstream call. api_key deliberately keeps the older \"empty means inherit\" rule instead, " + + "because losing a credential breaks the source while losing a proxy only changes its route.", + "durations": "timeout and queue_timeout are Go duration strings (\"300s\", \"5m\", \"1h\"); " + + "the empty string clears them back to the config defaults", "config_truth": "all configuration lives in config.yaml; API writes are persisted immediately", "credentials": "credentials are masked by default. GET /api/v1/sources/{name}?reveal=credentials " + "returns them in the clear and is admin-only — the Web UI edit dialog uses it, because a form " + @@ -294,23 +308,48 @@ func maskSources(srcs []config.Source) []map[string]interface{} { } func maskSource(s config.Source) map[string]interface{} { - return map[string]interface{}{ + return maskSourceFields(s, false, false) +} + +// maskSourceFields builds the API view of a source. +// +// revealCredentials swaps the masked api_key for the real one — admin-only, and +// used by the WebUI edit dialog, whose form has to round-trip the whole source +// or saving an unrelated field would blank the key. +// +// exposeDurations adds the two durations as strings, matching what the write +// path accepts. It rides along with the credential reveal because both exist for +// the same reason: config.Source tags Timeout/QueueTimeout `json:"-"`, so a +// plain marshal of the struct omits them. A form that cannot SEE the stored +// timeout would clear it on every save, since the timeout box is always sent. +func maskSourceFields(s config.Source, revealCredentials, exposeDurations bool) map[string]interface{} { + key := maskKey(s.APIKey) + if revealCredentials { + key = s.APIKey + } + m := map[string]interface{}{ "name": s.Name, "base_url": s.BaseURL, "adapter": s.Adapter, "endpoint": s.Endpoint, "image_endpoint": s.ImageEndpoint, - "api_key": maskKey(s.APIKey), + "api_key": key, "api_key_set": s.APIKey != "", "models": s.Models, "headers": maskHeaders(s.Headers), "proxy_url": s.ProxyURL, + "api_key_env": s.APIKeyEnv, "meta": s.Meta, "temperature": s.Temperature, "max_tokens": s.MaxTokens, "max_concurrent": s.MaxConcurrent, "rpm": s.RPM, } + if exposeDurations { + m["timeout"] = s.Timeout.String() + m["queue_timeout"] = s.QueueTimeout.String() + } + return m } func maskKey(k string) string { diff --git a/internal/gateway/source_fields_test.go b/internal/gateway/source_fields_test.go new file mode 100644 index 0000000..c28b96a --- /dev/null +++ b/internal/gateway/source_fields_test.go @@ -0,0 +1,495 @@ +package gateway + +import ( + "encoding/json" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "llmsproxy/internal/config" + "llmsproxy/internal/core" +) + +// This file covers the second round of the same class of defect bf0657b fixed. +// +// bf0657b stopped a partial source edit from clobbering api_key. The upsert +// still rewrites the WHOLE source, so every other field the request could not +// express was reset to its zero value on any edit: proxy_url, api_key_env, +// timeout and queue_timeout. +// +// The four are worse than cosmetic because they change how the source dials out. +// proxy_url silently turns a proxied upstream into a direct one (or vice versa) +// and api_key_env turns a source whose credential lives in an environment +// variable into a credential-less source — which keeps answering 200 on write +// and only fails on the NEXT call upstream, long after the editor left. +// +// The trigger is not exotic scripting: the WebUI's own edit form posts exactly +// such a payload. See TestWebUIEditPayloadPreservesRoutingFields. + +// richGateway writes a config.yaml whose source carries all four fragile +// fields, then boots a gateway over it. +func richGateway(t *testing.T) *Gateway { + t.Helper() + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + cfg := `listen: 127.0.0.1:8080 +gateway_keys: + - sk-test +default_model: AUTO +adapter_dir: ` + filepath.Join(dir, "adapters") + ` +runtime_file: ` + filepath.Join(dir, "runtime.json") + ` +sources: + - name: rich + base_url: https://api.deepseek.com + api_key: sk-live + adapter: deepseek + proxy_url: http://127.0.0.1:7890 + timeout: 300s + queue_timeout: 90s + max_concurrent: 8 + models: + - id: deepseek-v4-flash + priority: 100 + kind: chat +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0600); err != nil { + t.Fatal(err) + } + c, err := config.Load(cfgPath) + if err != nil { + t.Fatal(err) + } + core, err := core.NewFromConfig(c) + if err != nil { + t.Fatalf("core: %v", err) + } + t.Cleanup(core.Close) + g, err := New(core) + if err != nil { + t.Fatal(err) + } + return g +} + +// sourceField reads one field off the live source, i.e. what the routes +// actually use rather than what the file happens to contain. +func sourceField(t *testing.T, g *Gateway, name string, get func(config.Source) interface{}) interface{} { + t.Helper() + for _, s := range g.core.Sources() { + if s.Name == name { + return get(s) + } + } + t.Fatalf("source %q not found", name) + return nil +} + +// webUIPayload is byte-for-byte the JSON that index.html's saveSource() builds +// from the edit dialog. It deliberately carries none of the four fragile +// fields, because the dialog has no inputs for them. +const webUIPayload = `{ + "name":"rich","base_url":"https://api.deepseek.com","api_key":"sk-live", + "adapter":"deepseek","endpoint":"","image_endpoint":"", + "models":[{"id":"deepseek-v4-flash","priority":100,"kind":"chat"}], + "headers":{},"meta":{},"temperature":0,"max_tokens":0,"max_concurrent":16,"rpm":0 +}` + +// TestWebUIEditPayloadPreservesRoutingFields is the regression test for the +// reported defect: changing max_concurrent from 8 to 16 in the UI must not +// silently drop the proxy, the env-var credential, or the timeouts. +func TestWebUIEditPayloadPreservesRoutingFields(t *testing.T) { + g := richGateway(t) + + // The one field the operator actually changed. + rr := doReq(t, g, "POST", "/api/sources", webUIPayload) + if rr.Code != 200 { + t.Fatalf("edit status=%d body=%s", rr.Code, rr.Body.String()) + } + + checks := []struct { + field string + got interface{} + want interface{} + }{ + {"proxy_url", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.ProxyURL }), + "http://127.0.0.1:7890"}, + {"timeout", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.Timeout }), + 300 * time.Second}, + {"queue_timeout", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.QueueTimeout }), + 90 * time.Second}, + // The field the edit DID change, so a fix that simply refuses to write + // anything would also pass the three assertions above. + {"max_concurrent", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.MaxConcurrent }), + 16}, + } + for _, c := range checks { + if c.got != c.want { + t.Errorf("%s = %v, want %v", c.field, c.got, c.want) + } + } +} + +// TestSourceEditPreservesAPIKeyEnv is called out separately because it is the +// only one of the four whose loss is a credential failure rather than a routing +// difference, and it is invisible until the next upstream call. +func TestSourceEditPreservesAPIKeyEnv(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + cfg := `listen: 127.0.0.1:8080 +gateway_keys: + - sk-test +default_model: AUTO +adapter_dir: ` + filepath.Join(dir, "adapters") + ` +runtime_file: ` + filepath.Join(dir, "runtime.json") + ` +sources: + - name: envonly + base_url: https://api.deepseek.com + api_key_env: DEEPSEEK_KEY + adapter: deepseek + max_concurrent: 8 + models: + - id: deepseek-v4-flash + priority: 100 + kind: chat +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0600); err != nil { + t.Fatal(err) + } + c, err := config.Load(cfgPath) + if err != nil { + t.Fatal(err) + } + core, err := core.NewFromConfig(c) + if err != nil { + t.Fatalf("core: %v", err) + } + t.Cleanup(core.Close) + g, err := New(core) + if err != nil { + t.Fatal(err) + } + + // A credential-less source is legitimate, so the API must not invent one + // here; the point is that the ENV reference survives the edit. + rr := doReq(t, g, "POST", "/api/sources", `{ + "name":"envonly","base_url":"https://api.deepseek.com","api_key":"", + "adapter":"deepseek","models":[{"id":"deepseek-v4-flash","priority":100,"kind":"chat"}], + "headers":{},"meta":{},"max_concurrent":4}`) + if rr.Code != 200 { + t.Fatalf("edit status=%d body=%s", rr.Code, rr.Body.String()) + } + got := sourceField(t, g, "envonly", func(s config.Source) interface{} { return s.APIKeyEnv }) + if got != "DEEPSEEK_KEY" { + t.Fatalf("api_key_env = %v after an edit that did not mention it; "+ + "the source now has no credential and will only fail on the next upstream call", got) + } + if n := sourceField(t, g, "envonly", func(s config.Source) interface{} { return s.MaxConcurrent }); n != 4 { + t.Fatalf("max_concurrent = %v, want the edited 4", n) + } +} + +// TestSourceEditPersistsInheritedFieldsToDisk: inheriting in memory is not +// enough — the values must reach config.yaml, or a restart silently undoes the +// fix and the field is lost anyway. +func TestSourceEditPersistsInheritedFieldsToDisk(t *testing.T) { + g := richGateway(t) + if rr := doReq(t, g, "POST", "/api/sources", webUIPayload); rr.Code != 200 { + t.Fatalf("edit status=%d: %s", rr.Code, rr.Body.String()) + } + raw, err := os.ReadFile(g.core.Config().Path) + if err != nil { + t.Fatalf("read config: %v", err) + } + text := string(raw) + for _, want := range []string{ + "proxy_url: http://127.0.0.1:7890", + } { + if !strings.Contains(text, want) { + t.Errorf("config.yaml lost %q on an unrelated edit:\n%s", want, text) + } + } + // The durations are re-serialized in Go's canonical form (300s becomes + // 5m0s), so they must be checked SEMANTICALLY. Asserting the original + // spelling would be a false red: the value survived, only its text changed. + // Re-loading the file is also the only way to prove the persisted value + // parses back to the same duration. + reloaded, err := config.Load(g.core.Config().Path) + if err != nil { + t.Fatalf("re-read config: %v", err) + } + if len(reloaded.Sources) == 0 { + t.Fatal("no sources after reload") + } + s := reloaded.Sources[0] + if s.Timeout != 300*time.Second { + t.Errorf("persisted timeout = %v, want 300s", s.Timeout) + } + if s.QueueTimeout != 90*time.Second { + t.Errorf("persisted queue_timeout = %v, want 90s", s.QueueTimeout) + } + if s.ProxyURL != "http://127.0.0.1:7890" { + t.Errorf("persisted proxy_url = %v, want it preserved", s.ProxyURL) + } +} + +// ---- explicit set / clear, i.e. the pointer semantics actually work ---- + +func TestSourceOptionalFieldsCanBeSet(t *testing.T) { + g := richGateway(t) + rr := doReq(t, g, "POST", "/api/sources", `{ + "name":"rich","base_url":"https://api.deepseek.com","api_key":"sk-live", + "adapter":"deepseek","models":[{"id":"deepseek-v4-flash","priority":100,"kind":"chat"}], + "headers":{},"meta":{}, + "proxy_url":"http://127.0.0.1:1080", + "api_key_env":"NEW_ENV_KEY", + "timeout":"45s", + "queue_timeout":"5s"}`) + if rr.Code != 200 { + t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String()) + } + for _, c := range []struct { + field string + got interface{} + want interface{} + }{ + {"proxy_url", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.ProxyURL }), "http://127.0.0.1:1080"}, + {"api_key_env", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.APIKeyEnv }), "NEW_ENV_KEY"}, + {"timeout", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.Timeout }), 45 * time.Second}, + {"queue_timeout", sourceField(t, g, "rich", func(s config.Source) interface{} { return s.QueueTimeout }), 5 * time.Second}, + } { + if c.got != c.want { + t.Errorf("%s = %v, want %v", c.field, c.got, c.want) + } + } +} + +// TestSourceOptionalFieldsCanBeCleared is the half that an "empty means +// inherit" implementation gets wrong: it can never clear a field, so emptying +// the proxy box in the UI would keep using the old proxy forever. +func TestSourceOptionalFieldsCanBeCleared(t *testing.T) { + g := richGateway(t) + rr := doReq(t, g, "POST", "/api/sources", `{ + "name":"rich","base_url":"https://api.deepseek.com","api_key":"sk-live", + "adapter":"deepseek","models":[{"id":"deepseek-v4-flash","priority":100,"kind":"chat"}], + "headers":{},"meta":{}, + "proxy_url":"","timeout":"","queue_timeout":""}`) + if rr.Code != 200 { + t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String()) + } + if got := sourceField(t, g, "rich", func(s config.Source) interface{} { return s.ProxyURL }); got != "" { + t.Errorf("proxy_url = %v, want it cleared", got) + } + // A cleared timeout falls back to the config default (120s), which is what + // mergedSources applies for a zero value — assert the effective behaviour + // rather than the raw zero, since that is what a caller actually gets. + if got := sourceField(t, g, "rich", func(s config.Source) interface{} { return s.Timeout }); got != config.DefaultSourceTimeout { + t.Errorf("timeout = %v, want the default %v after clearing", got, config.DefaultSourceTimeout) + } + // api_key_env was NOT in the request, so it must be untouched even though + // its sibling fields were cleared. + _ = rr +} + +// TestSourceOptionalFieldsClearIsScoped: clearing the proxy must not clear the +// env credential that the same request did not mention. +func TestSourceOptionalFieldsClearIsScoped(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + cfg := `listen: 127.0.0.1:8080 +gateway_keys: + - sk-test +default_model: AUTO +adapter_dir: ` + filepath.Join(dir, "adapters") + ` +runtime_file: ` + filepath.Join(dir, "runtime.json") + ` +sources: + - name: both + base_url: https://api.deepseek.com + api_key_env: DEEPSEEK_KEY + adapter: deepseek + proxy_url: http://127.0.0.1:7890 + max_concurrent: 8 + models: + - id: deepseek-v4-flash + priority: 100 + kind: chat +` + os.WriteFile(cfgPath, []byte(cfg), 0600) + c, err := config.Load(cfgPath) + if err != nil { + t.Fatal(err) + } + core, err := core.NewFromConfig(c) + if err != nil { + t.Fatalf("core: %v", err) + } + t.Cleanup(core.Close) + g, err := New(core) + if err != nil { + t.Fatal(err) + } + rr := doReq(t, g, "POST", "/api/sources", `{ + "name":"both","base_url":"https://api.deepseek.com","api_key":"", + "adapter":"deepseek","models":[{"id":"deepseek-v4-flash","priority":100,"kind":"chat"}], + "headers":{},"meta":{},"proxy_url":""}`) + if rr.Code != 200 { + t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String()) + } + if got := sourceField(t, g, "both", func(s config.Source) interface{} { return s.APIKeyEnv }); got != "DEEPSEEK_KEY" { + t.Errorf("api_key_env = %v; clearing proxy_url must not touch it", got) + } + if got := sourceField(t, g, "both", func(s config.Source) interface{} { return s.ProxyURL }); got != "" { + t.Errorf("proxy_url = %v, want cleared", got) + } +} + +func TestSourceRejectsBadDuration(t *testing.T) { + g := richGateway(t) + before := sourceField(t, g, "rich", func(s config.Source) interface{} { return s.Timeout }) + rr := doReq(t, g, "POST", "/api/sources", `{ + "name":"rich","base_url":"https://api.deepseek.com","api_key":"sk-live", + "adapter":"deepseek","models":[{"id":"deepseek-v4-flash","priority":100,"kind":"chat"}], + "headers":{},"meta":{},"timeout":"not-a-duration"}`) + if rr.Code != 400 { + t.Fatalf("status=%d, want 400 for an unparseable duration: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "timeout") { + t.Errorf("the error must name the offending field: %s", rr.Body.String()) + } + // A rejected write must leave the source exactly as it was. + if after := sourceField(t, g, "rich", func(s config.Source) interface{} { return s.Timeout }); after != before { + t.Errorf("a rejected edit still changed timeout: %v -> %v", before, after) + } +} + +// TestSourceCreateSetsOptionalFields: the create path must accept them too, and +// must not require an existing source to inherit from (a nil-pointer crash +// here would only show up as a 500 on every new source). +func TestSourceCreateSetsOptionalFields(t *testing.T) { + up := mockUpstream() + defer up.Close() + g := newTestGateway(t) + rr := doReq(t, g, "POST", "/api/sources", `{ + "name":"fresh","base_url":"`+up.URL+`","api_key":"sk-x","adapter":"openai", + "models":[{"id":"m1","kind":"chat"}],"headers":{},"meta":{}, + "proxy_url":"http://127.0.0.1:7890","timeout":"10s","queue_timeout":"3s"}`) + if rr.Code != 200 { + t.Fatalf("create status=%d body=%s", rr.Code, rr.Body.String()) + } + if got := sourceField(t, g, "fresh", func(s config.Source) interface{} { return s.ProxyURL }); got != "http://127.0.0.1:7890" { + t.Errorf("proxy_url = %v, want it set on create", got) + } + if got := sourceField(t, g, "fresh", func(s config.Source) interface{} { return s.Timeout }); got != 10*time.Second { + t.Errorf("timeout = %v, want 10s on create", got) + } +} + +// TestSourceRevealExposesDurations guards the round-trip the edit form depends +// on. config.Source tags Timeout/QueueTimeout `json:"-"`, so marshaling the +// struct omits them; the form then reads an empty timeout box and — because it +// always sends the box back — CLEARS the stored timeout on any unrelated save. +// A test that only checks "the edit preserves timeout" would not have caught +// that, because the loss happens between the read and the write. +// +// The credentials are asserted in the same breath: they came from the same +// branch, and an earlier iteration of this fix masked the key and broke the +// form's other half. +func TestSourceRevealExposesDurations(t *testing.T) { + up := mockUpstream() + defer up.Close() + g := newTestGateway(t, config.Source{ + Name: "revealed", + BaseURL: up.URL, + Adapter: "openai", + APIKey: "sk-reveal-me", + Models: []config.Model{{ID: "m1", Kind: "chat"}}, + Timeout: 300 * time.Second, + }) + + rr := doReq(t, g, "GET", "/api/v1/sources/revealed?reveal=credentials", "") + if rr.Code != 200 { + t.Fatalf("reveal status=%d: %s", rr.Code, rr.Body.String()) + } + var view struct { + Source map[string]interface{} `json:"source"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &view); err != nil { + t.Fatalf("decode: %v", err) + } + // Duration strings, matching what the write path parses. + if got := view.Source["timeout"]; got != "5m0s" { + t.Errorf("reveal timeout = %v, want the string \"5m0s\"; the edit form "+ + "would show an empty box and clear the stored value on save", got) + } + // The unset one shows the DEFAULT, not "": ApplyDefaults filled + // QueueTimeout before the form ever sees it. Both are acceptable as long as + // the value round-trips, so assert it is a parseable duration string. + qs, _ := view.Source["queue_timeout"].(string) + if _, err := time.ParseDuration(qs); err != nil { + t.Errorf("reveal queue_timeout = %q is not a duration string: %v", qs, err) + } + if got := view.Source["api_key"]; got != "sk-reveal-me" { + t.Errorf("reveal api_key = %v, want it in the clear for the form", got) + } + // The masked view must NOT gain the durations' secrets — it is fine for it + // to omit them, but it must never carry the key. + masked := doReq(t, g, "GET", "/api/v1/sources/revealed", "") + if strings.Contains(masked.Body.String(), "sk-reveal-me") { + t.Error("the masked view leaked the key") + } +} + +// TestSourcePayloadCoversEveryEditableField is the guard against the class +// itself. Any config.Source field a source edit can legitimately set must +// exist on sourcePayload, otherwise the upsert resets it. +// +// It reflects over the STRUCT rather than over json.Marshal output: the four +// pointer fields carry `omitempty`, so a nil pointer legitimately disappears +// from the marshaled form — that absence is precisely the "not mentioned" signal +// the handler relies on. Checking marshaled keys would therefore report a +// healthy field as missing. +func TestSourcePayloadCoversEveryEditableField(t *testing.T) { + // Fields of config.Source that a source edit can legitimately set. + want := []string{ + "name", "base_url", "api_key", "api_key_env", "adapter", "endpoint", + "image_endpoint", "models", "headers", "proxy_url", "meta", + "temperature", "max_tokens", "max_concurrent", "rpm", + "timeout", "queue_timeout", + } + pt := reflect.TypeOf(sourcePayload{}) + for _, f := range want { + if _, ok := pt.FieldByName(f); !ok { + // Tolerate a differently-spelled Go field only when the JSON tag + // matches, so the check stays about the wire contract. + if !hasJSONTag(pt, f) { + t.Errorf("config.Source has %q but sourcePayload has no such JSON key: "+ + "an edit that omits it resets the stored value", f) + } + } + } +} + +// hasJSONTag reports whether any field of t carries the given JSON name. +func hasJSONTag(t reflect.Type, name string) bool { + for i := 0; i < t.NumField(); i++ { + if jsonName(t.Field(i)) == name { + return true + } + } + return false +} + +// jsonName returns the JSON name of a struct field, falling back to the Go +// field name when no tag is present. +func jsonName(f reflect.StructField) string { + tag := f.Tag.Get("json") + if tag == "" { + return f.Name + } + if n := strings.Split(tag, ",")[0]; n != "" { + return n + } + return f.Name +} diff --git a/internal/gateway/ui/index.html b/internal/gateway/ui/index.html index 0187339..e8c5c58 100644 --- a/internal/gateway/ui/index.html +++ b/internal/gateway/ui/index.html @@ -904,6 +904,12 @@ mConc: "并发上限", mRPM: "RPM 限速 (0=不限)", mTemp: "温度", + mKeyEnv: "Key 环境变量", + mKeyEnvPh: "优先于 API Key,不落盘明文", + mProxy: "代理 URL", + mProxyPh: "如 http://127.0.0.1:7890,留空直连", + mTimeout: "请求超时", + mTimeoutPh: "如 300s,留空用默认 120s", mModels: "模型列表", mAddModel: "+ 模型", mMeta: "Meta", @@ -1135,6 +1141,12 @@ mConc: "Max concurrency", mRPM: "RPM limit (0 = unlimited)", mTemp: "Temperature", + mKeyEnv: "Key env var", + mKeyEnvPh: "Takes precedence over API Key; nothing written to disk", + mProxy: "Proxy URL", + mProxyPh: "e.g. http://127.0.0.1:7890 — empty means direct", + mTimeout: "Request timeout", + mTimeoutPh: "e.g. 300s — empty uses the 120s default", mModels: "Models", mAddModel: "+ model", mMeta: "Meta", @@ -2866,7 +2878,11 @@