fix(sources): implement PUT and stop partial edits from clobbering api_key

Two defects on the admin source write path, both found while adding a model to a live source by hand. PUT /api/sources/{name} was advertised in the API index but never implemented — handleSourcesAPI only switched on GET/POST/DELETE, so the documented update verb answered 405 while the POST upsert behind it worked. POST is an upsert that replaces the whole source, so a partial edit that did not carry api_key persisted an empty or placeholder credential. The source kept its name, base_url and models, the write returned 200, and the source then answered 401 on the next request — long after the writing script exited 0. The WebUI had been routing around this by loading the real key through ?reveal=credentials; any script or partial update went straight into it. - implement PUT, taking the name from the path and rejecting a body name that disagrees rather than silently resolving to one of them - inherit the stored credential when api_key is omitted or sent as the literal "__KEEP__"; an explicit new key still rotates - an empty api_key on a source that does not exist yet stays empty, since credential-less local upstreams are legitimate - add model_ids, an additive shorthand, so "add these models" never has to read and echo the existing list back - align the API index with the implementation The model_ids merge had a first cut that dropped the existing list when the request carried no models field; TestSourceModelIDsIsAdditive caught it. Verified by mutation: removing PUT turns three tests red, flattening resolveAPIKey into a pass-through turns TestSourceUpsertKeepsAPIKey red on both subtests, and making model_ids replace instead of merge turns TestSourceModelIDsIsAdditive red.
2026-10-03 23:54:06 +00:00 · 2026-10-01 18:15:29 +08:00
parent 3548745f98
commit a1d4365515
3 changed files with 365 additions and 6 deletions
--- a/internal/gateway/api.go
+++ b/internal/gateway/api.go
@ -78,6 +78,62 @@ type sourcePayload struct {
 	MaxTokens     int                    `json:"max_tokens"`
 	MaxConcurrent int                    `json:"max_concurrent"`
 	RPM           int                    `json:"rpm"` // optional requests-per-minute cap, 0 = unlimited
+	// ModelIDs is an additive-only shorthand: when set, the named models are
+	// merged into an existing source instead of Models replacing the list.
+	// 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"`
+}
+
+// keepExistingAPIKey is the mask a client sends when it means "keep the
+// current credential". POST /api/sources is an upsert, so a script that edits
+// one field and echoes a placeholder back would otherwise persist that
+// placeholder as the live api_key — which fails closed as 401 on the next
+// request, long after the script exited successfully. See TestSourceUpsertKeepsAPIKey.
+const keepExistingAPIKey = "__KEEP__"
+
+// resolveAPIKey returns the credential to store, inheriting the current one
+// when the payload does not supply a new one.
+//
+// Two distinct "no new credential" signals exist and they must not collapse:
+//   - the explicit mask keepExistingAPIKey — always inherit.
+//   - the empty string — inherit only for a source that already has a
+//     credential. Empty is a legitimate value for a *new* credential-less
+//     source (dshcodebuddy uses "dsh-local-noauth", local servers use ""),
+//     so a fresh source with an empty key must stay empty rather than inherit
+//     nothing and fail differently.
+func (g *Gateway) resolveAPIKey(name, want string, exists bool) string {
+	if want == keepExistingAPIKey {
+		return g.currentAPIKey(name)
+	}
+	if exists && want == "" {
+		return g.currentAPIKey(name)
+	}
+	return want
+}
+
+func (g *Gateway) currentAPIKey(name string) string {
+	if s := g.sourceByName(name); s != nil {
+		return s.APIKey
+	}
+	return ""
+}
+
+// sourceByName returns a live view of one source, or nil when it does not
+// exist. It goes through core.Sources() so the key is already unsealed and
+// defaults applied, matching what the routes actually use.
+func (g *Gateway) sourceByName(name string) *config.Source {
+	for _, s := range g.core.Sources() {
+		if s.Name == name {
+			ss := s
+			return &ss
+		}
+	}
+	return nil
+}
+
+func (g *Gateway) sourceExists(name string) bool {
+	return g.sourceByName(name) != nil
 }

 func (g *Gateway) handleSourcesAPI(w http.ResponseWriter, r *http.Request) {
@ -91,17 +147,34 @@ func (g *Gateway) handleSourcesAPI(w http.ResponseWriter, r *http.Request) {
 	switch r.Method {
 	case http.MethodGet:
 		writeJSON(w, http.StatusOK, map[string]interface{}{"sources": g.core.Sources()})
-	case http.MethodPost:
+	case http.MethodPost, http.MethodPut:
 		var p sourcePayload
 		body, _ := io.ReadAll(r.Body)
 		if err := json.Unmarshal(body, &p); err != nil {
 			writeError(w, http.StatusBadRequest, "invalid_request", "invalid json: "+err.Error())
 			return
 		}
+		// PUT addresses the source by path; the body name is only a fallback so
+		// a single-field edit does not have to echo the name back. A body that
+		// disagrees with the path is a caller bug, not something to silently
+		// resolve — it usually means the wrong source is about to be written.
+		if r.Method == http.MethodPut {
+			if path == "" {
+				writeError(w, http.StatusBadRequest, "invalid_request", "source name required in path")
+				return
+			}
+			if p.Name != "" && p.Name != path {
+				writeError(w, http.StatusBadRequest, "invalid_request",
+					"source name in body ("+p.Name+") does not match path ("+path+")")
+				return
+			}
+			p.Name = path
+		}
+		exists := g.sourceExists(p.Name)
 		src := config.Source{
 			Name:          p.Name,
 			BaseURL:       p.BaseURL,
-			APIKey:        p.APIKey,
+			APIKey:        g.resolveAPIKey(p.Name, p.APIKey, exists),
 			Adapter:       p.Adapter,
 			Endpoint:      p.Endpoint,
 			ImageEndpoint: p.ImageEndpoint,
@ -113,6 +186,29 @@ func (g *Gateway) handleSourcesAPI(w http.ResponseWriter, r *http.Request) {
 			MaxConcurrent: p.MaxConcurrent,
 			RPM:           p.RPM,
 		}
+		// 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
+		// request to empty the list -- that distinction is the whole point.
+		if len(p.ModelIDs) > 0 {
+			if len(src.Models) == 0 {
+				if existing := g.sourceByName(p.Name); existing != nil {
+					src.Models = append(src.Models, existing.Models...)
+				}
+			}
+			have := map[string]bool{}
+			for _, m := range src.Models {
+				have[m.ID] = true
+			}
+			for _, id := range p.ModelIDs {
+				id = strings.TrimSpace(id)
+				if id == "" || have[id] {
+					continue
+				}
+				have[id] = true
+				src.Models = append(src.Models, config.Model{ID: id, Kind: "chat"})
+			}
+		}
 		if err := g.core.AddSource(src); err != nil {
 			writeError(w, http.StatusBadRequest, "source_error", err.Error())
 			return
--- a/internal/gateway/apiv1.go
+++ b/internal/gateway/apiv1.go
@ -165,7 +165,7 @@ func (g *Gateway) apiV1Index(w http.ResponseWriter, r *http.Request) {
 			{Method: "POST", Path: "/api/sources", Auth: "admin", Summary: "add or replace a source",
 				WriteEffect: "writes config.yaml (api_key sealed at rest)"},
 			{Method: "PUT", Path: "/api/sources/{name}", Auth: "admin", Summary: "update one source",
-				WriteEffect: "writes config.yaml"},
+				WriteEffect: "writes config.yaml (api_key sealed at rest)"},
 			{Method: "DELETE", Path: "/api/sources/{name}", Auth: "admin", Summary: "delete a source",
 				WriteEffect: "writes config.yaml"},

@ -187,9 +187,13 @@ func (g *Gateway) apiV1Index(w http.ResponseWriter, r *http.Request) {
 			{Method: "POST", Path: "/v1/chat/completions", Auth: "any", Summary: "OpenAI-compatible inference"},
 		},
 		"conventions": map[string]interface{}{
-			"errors":       "{ \"error\": { \"type\": <code>, \"message\": <text> } }",
-			"path_escape":  "URL-encode source and key names; {name} is a single path segment",
-			"idempotency":  "POST /api/sources and PUT /api/sources/{name} both upsert by name",
+			"errors":      "{ \"error\": { \"type\": <code>, \"message\": <text> } }",
+			"path_escape": "URL-encode source and key names; {name} is a single path segment",
+			"idempotency": "POST /api/sources and PUT /api/sources/{name} both upsert by name; " +
+				"PUT takes the name from the path and rejects a body name that disagrees",
+			"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",
 			"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 " +
--- a/internal/gateway/sources_api_test.go
+++ b/internal/gateway/sources_api_test.go
@ -0,0 +1,259 @@
+package gateway
+
+import (
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"strings"
+	"testing"
+
+	"llmsproxy/internal/config"
+)
+
+// These cover the two write-path defects found while adding a model to a live
+// source by hand:
+//
+//  1. PUT /api/sources/{name} was documented in the API index but not
+//     implemented, so it answered 405 while POST (which does the same upsert)
+//     worked. A caller following the documented contract could not edit a
+//     source at all.
+//  2. POST is an upsert that replaces the whole source, so a partial edit that
+//     did not carry api_key persisted an empty/placeholder credential. The
+//     source kept its name and models and answered 401 on the next call, well
+//     after the writing script had exited 0.
+//
+// The second one is the dangerous class: a write that reports success and
+// breaks the source later.
+
+// sourceAPIKey reads the stored key back the way an operator would, i.e. what
+// the routes actually use, not what the file happens to contain.
+func sourceAPIKey(t *testing.T, g *Gateway, name string) string {
+	t.Helper()
+	for _, s := range g.core.Sources() {
+		if s.Name == name {
+			return s.APIKey
+		}
+	}
+	t.Fatalf("source %q not found", name)
+	return ""
+}
+
+func seededGateway(t *testing.T) (*Gateway, string) {
+	t.Helper()
+	up := mockUpstream()
+	t.Cleanup(up.Close)
+	g := newTestGateway(t, config.Source{
+		Name:    "seeded",
+		BaseURL: up.URL,
+		Adapter: "openai",
+		APIKey:  "sk-real-credential",
+		Models:  []config.Model{{ID: "m-one", Kind: "chat"}},
+	})
+	return g, up.URL
+}
+
+func TestSourcePutUpdatesSource(t *testing.T) {
+	g, up := seededGateway(t)
+
+	// A single-field edit: change one model's priority, name the source only in
+	// the path. This is the shape the API index documents for PUT.
+	rr := doReq(t, g, "PUT", "/api/sources/seeded", fmt.Sprintf(
+		`{"base_url":%q,"adapter":"openai","api_key":"sk-real-credential","models":[{"id":"m-one","priority":42,"kind":"chat"}]}`, up))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("PUT status=%d body=%s", rr.Code, rr.Body.String())
+	}
+
+	var found bool
+	for _, s := range g.core.Sources() {
+		if s.Name != "seeded" {
+			continue
+		}
+		found = true
+		for _, m := range s.Models {
+			if m.ID == "m-one" && m.Priority != 42 {
+				t.Fatalf("priority not applied: %+v", s.Models)
+			}
+		}
+	}
+	if !found {
+		t.Fatal("source vanished after PUT")
+	}
+	// And it must be routable, not merely stored.
+	if rr := doReq(t, g, "GET", "/v1/models", ""); rr.Code != http.StatusOK ||
+		!strings.Contains(rr.Body.String(), "m-one") {
+		t.Fatalf("model not routable after PUT: %d %s", rr.Code, rr.Body.String())
+	}
+}
+
+func TestSourcePutRejectsNameMismatch(t *testing.T) {
+	g, _ := seededGateway(t)
+	// Body names a different source than the path. Silently preferring either
+	// one would let a client edit a source it did not name.
+	rr := doReq(t, g, "PUT", "/api/sources/seeded", `{"name":"other","base_url":"http://x","models":[{"id":"m"}]}`)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("name mismatch accepted: status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	if sourceAPIKey(t, g, "seeded") != "sk-real-credential" {
+		t.Fatal("rejected PUT still mutated the source")
+	}
+}
+
+func TestSourcePutRequiresNameInPath(t *testing.T) {
+	g, _ := seededGateway(t)
+	rr := doReq(t, g, "PUT", "/api/sources", `{"base_url":"http://x","models":[{"id":"m"}]}`)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("PUT without a name in the path: status=%d body=%s", rr.Code, rr.Body.String())
+	}
+}
+
+func TestSourceUpsertKeepsAPIKey(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		sentKey string
+	}{
+		{"explicit mask", keepExistingAPIKey},
+		{"omitted key", ""},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			g, up := seededGateway(t)
+			rr := doReq(t, g, "PUT", "/api/sources/seeded", fmt.Sprintf(
+				`{"base_url":%q,"adapter":"openai","api_key":%q,"models":[{"id":"m-one"},{"id":"m-two"}]}`,
+				up, tc.sentKey))
+			if rr.Code != http.StatusOK {
+				t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String())
+			}
+			if got := sourceAPIKey(t, g, "seeded"); got != "sk-real-credential" {
+				t.Fatalf("api_key was clobbered: got %q, want the pre-existing credential", got)
+			}
+			// The failure this guards is a source that looks configured but
+			// cannot authenticate, so assert the route still works.
+			if rr := doReq(t, g, "GET", "/v1/models", ""); rr.Code != http.StatusOK {
+				t.Fatalf("source unusable after edit: %d %s", rr.Code, rr.Body.String())
+			}
+		})
+	}
+}
+
+func TestSourceUpsertStillWritesNewKey(t *testing.T) {
+	// The keep-existing path must not swallow a deliberate rotation.
+	g, up := seededGateway(t)
+	rr := doReq(t, g, "PUT", "/api/sources/seeded", fmt.Sprintf(
+		`{"base_url":%q,"adapter":"openai","api_key":"sk-rotated","models":[{"id":"m-one"}]}`, up))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	if got := sourceAPIKey(t, g, "seeded"); got != "sk-rotated" {
+		t.Fatalf("rotation ignored: got %q", got)
+	}
+}
+
+func TestSourceUpsertNewSourceMayHaveNoKey(t *testing.T) {
+	// Empty is a real value for a credential-less source (local upstreams), so
+	// inherit-on-empty must not invent one for a source that does not exist yet.
+	up := mockUpstream()
+	defer up.Close()
+	g := newTestGateway(t)
+	rr := doReq(t, g, "POST", "/api/sources", fmt.Sprintf(
+		`{"name":"noauth","base_url":%q,"adapter":"openai","models":[{"id":"local-m"}]}`, up.URL))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	if got := sourceAPIKey(t, g, "noauth"); got != "" {
+		t.Fatalf("unexpected key on a new credential-less source: %q", got)
+	}
+}
+
+func TestSourceModelIDsIsAdditive(t *testing.T) {
+	// "Add one model" is the edit that motivated the defect: it needs no read
+	// of the existing list, so it must not require one.
+	g, up := seededGateway(t)
+	rr := doReq(t, g, "PUT", "/api/sources/seeded", fmt.Sprintf(
+		`{"base_url":%q,"adapter":"openai","api_key":%q,"model_ids":["m-two","m-two"," m-three "]}`, up, keepExistingAPIKey))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	var ids []string
+	for _, s := range g.core.Sources() {
+		if s.Name == "seeded" {
+			for _, m := range s.Models {
+				ids = append(ids, m.ID)
+			}
+		}
+	}
+	// m-one kept, m-two not duplicated, blank entries dropped, whitespace trimmed.
+	want := map[string]int{"m-one": 1, "m-two": 1, "m-three": 1}
+	if len(ids) != len(want) {
+		t.Fatalf("models=%v, want exactly %v", ids, want)
+	}
+	seen := map[string]int{}
+	for _, id := range ids {
+		seen[id]++
+	}
+	for id, n := range want {
+		if seen[id] != n {
+			t.Fatalf("models=%v: %q appears %d times, want %d", ids, id, seen[id], n)
+		}
+	}
+}
+
+func TestSourceWriteRejectsNonAdmin(t *testing.T) {
+	g, up := seededGateway(t)
+	for _, m := range []string{"POST", "PUT", "DELETE"} {
+		req, _ := http.NewRequest(m, "/api/sources/seeded", strings.NewReader(fmt.Sprintf(
+			`{"name":"seeded","base_url":%q,"models":[{"id":"m"}]}`, up)))
+		req.Header.Set("Authorization", "Bearer sk-not-a-real-key")
+		req.Header.Set("Content-Type", "application/json")
+		rr := httptest.NewRecorder()
+		g.Handler().ServeHTTP(rr, req)
+		if rr.Code == http.StatusOK {
+			t.Fatalf("%s allowed for an unknown key: %s", m, rr.Body.String())
+		}
+	}
+	if sourceAPIKey(t, g, "seeded") != "sk-real-credential" {
+		t.Fatal("unauthenticated write mutated the source")
+	}
+}
+
+// TestSourceWritePersistsToDisk guards the difference between "the in-memory
+// registry accepted it" and "it will survive a restart". The api_key must be
+// sealed at rest, never the literal placeholder.
+func TestSourceWritePersistsToDisk(t *testing.T) {
+	g, up := seededGateway(t)
+	rr := doReq(t, g, "PUT", "/api/sources/seeded", fmt.Sprintf(
+		`{"base_url":%q,"adapter":"openai","api_key":%q,"models":[{"id":"m-one"}]}`, up, keepExistingAPIKey))
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status=%d body=%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)
+	if strings.Contains(text, keepExistingAPIKey) {
+		t.Fatal("the keep-mask was persisted as a literal api_key")
+	}
+	if strings.Contains(text, "sk-real-credential") {
+		t.Fatal("api_key stored in plaintext; it must be sealed at rest")
+	}
+	if !strings.Contains(text, "m-one") {
+		t.Fatal("source not persisted")
+	}
+	// The masked GET view must not leak the credential either.
+	rr = doReq(t, g, "GET", "/api/v1/sources/seeded", "")
+	if rr.Code != http.StatusOK {
+		t.Fatalf("get status=%d", rr.Code)
+	}
+	var view struct {
+		Source struct {
+			APIKey string `json:"api_key"`
+		} `json:"source"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &view); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if view.Source.APIKey == "sk-real-credential" {
+		t.Fatal("GET /api/v1/sources leaked the plaintext api_key")
+	}
+}