MCP servers: fields that can be emptied, addresses without passwords

From review. A server's description, timeout and rate can be emptied from
the console and the page (MCPServerRequest.Clear), where an empty field
used to leave them as they were. An address with a user and password in
it is refused (adminapi.CheckMCPAddress): the credential belongs in a
header, sealed, and the address is shown and audited as written. The
console accepts llmgw:admin and the page lets it be changed, address
fixed. A grant to a key that is not there is refused rather than said
done.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/cli/tui_mcp.go

@@ -33,6 +33,8 @@ type mcpState struct {
 	grants []mcpGrant
 	where  map[string]string
 	ids    map[string]store.LimitOwner
+	// keys are the IDs of the keys there are, for a grant to one.
+	keys map[string]bool
 }
 
 // mcpGrant is one level of the tree that grants tools, and which.
@@ -80,6 +82,13 @@ func (m consoleModel) fetchMCP() tea.Cmd {
 				msg.name(store.ScopeUser, u.ID, u.Email)
 			}
 		}
+		msg.keys = map[string]bool{}
+		if keys, err := s.AllKeys(ctx); err == nil {
+			for _, k := range keys {
+				msg.keys[k.ID] = true
+				msg.where[store.LimitOwner{Scope: store.ScopeKey, ID: k.ID}.CounterKey()] = k.Name
+			}
+		}
 		msg.grants = []mcpGrant{}
 		for _, l := range limits {
 			if l.AllowedTools == nil {
@@ -244,8 +253,8 @@ func (m *consoleModel) mcpAddForm() {
 			if name == "" || addr == "" {
 				return nil, fmt.Errorf("a name and an address")
 			}
-			if u, err := url.Parse(addr); err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
-				return nil, fmt.Errorf("the address is http(s)://host/path")
+			if err := adminapi.CheckMCPAddress(addr); err != nil {
+				return nil, err
 			}
 			secrets, err := mcpAuth(v[3], strings.TrimSpace(v[4]), v[5])
 			if err != nil {
@@ -350,8 +359,8 @@ func (m *consoleModel) mcpEditForm(s adminapi.MCPServerView) {
 			textF("rate", rate, "calls a minute per caller; 0 is no bound"),
 		},
 		submit: func(m *consoleModel, v []string) (tea.Cmd, error) {
-			if u, err := url.Parse(v[0]); err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
-				return nil, fmt.Errorf("the address is http(s)://host/path")
+			if err := adminapi.CheckMCPAddress(strings.TrimSpace(v[0])); err != nil {
+				return nil, err
 			}
 			timeout, err := atoiOr(v[2], 0)
 			if err != nil || timeout < 0 {
@@ -361,10 +370,18 @@ func (m *consoleModel) mcpEditForm(s adminapi.MCPServerView) {
 			if err != nil || rate < 0 {
 				return nil, fmt.Errorf("rate is calls a minute")
 			}
-			if rate == 0 && s.RatePerMinute > 0 {
-				rate = -1 // the API's way of taking the bound away
+			req := adminapi.MCPServerRequest{URL: strings.TrimSpace(v[0]), Description: v[1], TimeoutSeconds: timeout,
+				RatePerMinute: rate}
+			// An emptied field empties it, rather than leaving it as it was.
+			if strings.TrimSpace(v[1]) == "" {
+				req.Clear = append(req.Clear, "description")
+			}
+			if timeout == 0 {
+				req.Clear = append(req.Clear, "timeout")
+			}
+			if rate == 0 {
+				req.Clear = append(req.Clear, "rate")
 			}
-			req := adminapi.MCPServerRequest{URL: v[0], Description: v[1], TimeoutSeconds: timeout, RatePerMinute: rate}
 			return m.call("Change "+s.Name, http.MethodPut, mcpPath(s.Name), req, nil), nil
 		}})
 }
@@ -414,7 +431,7 @@ func (m *consoleModel) mcpGrantForm(s adminapi.MCPServerView) {
 			}
 			owner, ok := m.mcp.ids[v[0]+":"+strings.ToLower(which)]
 			if v[0] == "key" {
-				owner, ok = store.LimitOwner{Scope: store.ScopeKey, ID: which}, true
+				owner, ok = store.LimitOwner{Scope: store.ScopeKey, ID: which}, m.mcp.keys[which]
 			}
 			if !ok {
 				return nil, fmt.Errorf("no %s %q", v[0], which)

internal/control/adminapi/mcp.go

@@ -57,6 +57,9 @@ type MCPServerRequest struct {
 	// RatePerMinute bounds each caller's calls to its tools; a negative
 	// number takes the bound away.
 	RatePerMinute int `json:"rate_per_minute,omitempty"`
+	// Clear empties what an empty field above would leave as it is:
+	// "description", "timeout" (back to the default), "rate".
+	Clear []string `json:"clear,omitempty"`
 }
 
 // MCPTestView is what the server said when asked: its tools, or why not.
@@ -171,8 +174,8 @@ func MCPServerFrom(ctx context.Context, st MCPStore, name string, req MCPServerR
 	if req.URL != "" {
 		m.URL = req.URL
 	}
-	if u, err := url.Parse(m.URL); m.URL != AdminMCPAddress && (err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "") {
-		return m, errors.New("the server's url is an http:// or https:// address, or " + AdminMCPAddress + " for this cluster's own")
+	if err := CheckMCPAddress(m.URL); err != nil {
+		return m, err
 	}
 	if req.Description != "" {
 		m.Description = req.Description
@@ -209,9 +212,39 @@ func MCPServerFrom(ctx context.Context, st MCPStore, name string, req MCPServerR
 	case req.RatePerMinute < 0:
 		m.RatePerMinute = 0
 	}
+	for _, f := range req.Clear {
+		switch f {
+		case "description":
+			m.Description = ""
+		case "timeout":
+			m.Timeout = 0
+		case "rate":
+			m.RatePerMinute = 0
+		default:
+			return m, errors.New("clear names description, timeout or rate, not " + f)
+		}
+	}
 	return m, nil
 }
 
+// CheckMCPAddress refuses an address a server cannot be reached at: an
+// http(s) one, or this cluster's own. Not one with a user and password in
+// it: a credential is a header, kept sealed, and an address is shown and
+// recorded in the audit log as written.
+func CheckMCPAddress(addr string) error {
+	if addr == AdminMCPAddress {
+		return nil
+	}
+	u, err := url.Parse(addr)
+	if err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "" {
+		return errors.New("the server's url is an http:// or https:// address, or " + AdminMCPAddress + " for this cluster's own")
+	}
+	if u.User != nil {
+		return errors.New("the server's url has a user and password in it: give the credential as a header, which is kept sealed")
+	}
+	return nil
+}
+
 // testMCPServer asks a server for its tools, with its credentials, as /mcp
 // would.
 func (a *API) testMCPServer(ctx context.Context, m store.MCPServer) MCPTestView {

internal/web/admin_mcp.go

@@ -34,6 +34,7 @@ type MCPAdmin interface {
 	PutLimits(ctx context.Context, l store.Limits) error
 	GrantTools(ctx context.Context, scope store.Scope, id, server string, tools []string) (store.ToolGrant, error)
 	RevokeTools(ctx context.Context, scope store.Scope, id, server string) error
+	KeyByID(ctx context.Context, id string) (store.Key, error)
 	Tree(ctx context.Context) ([]store.TreeOrg, error)
 	Users(ctx context.Context, state store.UserState) ([]store.User, error)
 }
@@ -193,12 +194,20 @@ func (p *AccountPages) RegisterAdminMCP(mux *http.ServeMux, s MCPAdmin, local fu
 	mux.HandleFunc("POST /admin/mcp/{name}/edit", adminsOnly(func(w http.ResponseWriter, r *http.Request, admin store.User) {
 		l, name := langOf(r), r.PathValue("name")
 		timeout, rate, err := mcpBounds(l, r.FormValue("timeout"), r.FormValue("rate"))
-		if err == nil && rate == 0 {
-			rate = -1 // an empty rate takes the bound away
-		}
 		if err == nil {
-			err = put(r, admin, name, adminapi.MCPServerRequest{URL: strings.TrimSpace(r.FormValue("url")),
-				Description: strings.TrimSpace(r.FormValue("description")), TimeoutSeconds: timeout, RatePerMinute: rate})
+			req := adminapi.MCPServerRequest{URL: strings.TrimSpace(r.FormValue("url")),
+				Description: strings.TrimSpace(r.FormValue("description")), TimeoutSeconds: timeout, RatePerMinute: rate}
+			// An emptied field empties it, rather than leaving it as it was.
+			if req.Description == "" {
+				req.Clear = append(req.Clear, "description")
+			}
+			if timeout == 0 {
+				req.Clear = append(req.Clear, "timeout")
+			}
+			if rate == 0 {
+				req.Clear = append(req.Clear, "rate")
+			}
+			err = put(r, admin, name, req)
 		}
 		done(w, r, admin, "mcp server "+name, l.T("mcp.changed", name), err)
 	}))
@@ -357,7 +366,12 @@ func mcpResolve(ctx context.Context, s MCPAdmin, l *Lang, scope store.Scope, whi
 	}
 	switch scope {
 	case store.ScopeKey:
-		return which, which, nil
+		// A key that is not there would be granted nothing, and said granted.
+		k, err := s.KeyByID(ctx, which)
+		if err != nil {
+			return "", "", errors.New(l.T("mcp.err.no_such", which))
+		}
+		return k.ID, k.Name + " (" + k.Prefix + "…)", nil
 	case store.ScopeUser:
 		users, err := s.Users(ctx, "")
 		if err != nil {

internal/web/admin_mcp_test.go

@@ -12,6 +12,7 @@ import (
 	"slices"
 	"strings"
 	"testing"
+	"time"
 
 	"gitea.mynodes.xyz/ilker/llmgw/internal/mcpclient/mcptest"
 	"gitea.mynodes.xyz/ilker/llmgw/internal/secrets"
@@ -183,3 +184,26 @@ func TestAGrantBelowALevelThatNamesOtherToolsSaysSoAndKeepsWhatItHad(t *testing.
 		t.Errorf("the team's list is %v; it should keep hr's, which it saw from its organization", l.AllowedTools)
 	}
 }
+
+func TestAnMCPServersFieldsCanBeEmptiedAndItsAddressHoldsNoPassword(t *testing.T) {
+	mux, st, _ := mcpPage(t, mcpAdmin)
+	ctx := context.Background()
+	_ = st.PutMCPServer(ctx, store.MCPServer{Name: "hr", URL: "https://hr/mcp", Description: "people", Enabled: true,
+		Timeout: 30 * time.Second, RatePerMinute: 10})
+	mirrorPost(t, mux, "/admin/mcp/hr/edit", url.Values{"csrf": {"tok"}, "url": {"https://hr/mcp"}, "description": {""},
+		"timeout": {""}, "rate": {""}})
+	if m, _ := st.MCPServer(ctx, "hr"); m.Description != "" || m.Timeout != 0 || m.RatePerMinute != 0 {
+		t.Errorf("emptied fields kept: %+v", m)
+	}
+	body := mirrorPost(t, mux, "/admin/mcp/add", url.Values{"csrf": {"tok"}, "name": {"db"},
+		"url": {"https://admin:hunter2@db.internal/mcp"}, "auth": {"none"}}).Body.String()
+	if _, err := st.MCPServer(ctx, "db"); err == nil || !strings.Contains(body, "user and password in it") {
+		t.Errorf("an address with a password was kept: %s", body)
+	}
+	// A grant to a key that is not there is refused, not said done.
+	body = mirrorPost(t, mux, "/admin/mcp/hr/grant", url.Values{"csrf": {"tok"}, "scope": {"key"}, "which": {"key-nope"},
+		"tools": {"hr.*"}}).Body.String()
+	if !strings.Contains(body, "There is no key-nope") {
+		t.Errorf("a grant to no key: %s", body)
+	}
+}

internal/web/templates/admin_mcp.html

@@ -57,16 +57,16 @@
     <button type="submit">{{t "mcp.revoke" .Label}}</button></form>
   {{end}}{{end}}
 
-  {{if not .Local}}
   <h3>{{t "mcp.change"}}</h3>
   <form method="post" action="/admin/mcp/{{.Name}}/edit">
     <input type="hidden" name="csrf" value="{{$.CSRF}}">
-    <label>{{t "mcp.address"}} <input type="text" inputmode="url" name="url" value="{{.URL}}" required></label>
+    <label>{{t "mcp.address"}} <input type="text" inputmode="url" name="url" value="{{.URL}}" required{{if .Local}} readonly{{end}}></label>
     <label>{{t "mcp.about"}} <input type="text" name="description" value="{{.Description}}"></label>
     <label>{{t "mcp.timeout"}} <input type="number" name="timeout" min="0" value="{{if .TimeoutSeconds}}{{.TimeoutSeconds}}{{end}}"></label>
     <label>{{t "mcp.rate"}} <input type="number" name="rate" min="0" value="{{if .RatePerMinute}}{{.RatePerMinute}}{{end}}"></label>
     <button type="submit">{{t "mcp.save"}}</button>
   </form>
+  {{if not .Local}}
   <h3>{{t "mcp.credential"}}</h3>
   <form method="post" action="/admin/mcp/{{.Name}}/credential">
     <input type="hidden" name="csrf" value="{{$.CSRF}}">