fix(security): shared agent-token operation policy — REST server mutations 403 for agents; index/search honors quarantine (#878, #877)

docs/api/rest-api.md

@@ -29,6 +29,16 @@ curl "http://127.0.0.1:8080/api/v1/servers?apikey=your-api-key"
 
 **Note:** Unix socket connections bypass API key authentication (OS-level auth).
 
+### Admin key vs. agent tokens
+
+Two kinds of credential authenticate against the REST API:
+
+- **Admin API key** — full access to every endpoint.
+- **Agent tokens** (`mcp_agt_` prefix, see [Agent Tokens](../features/agent-tokens.md)) — scoped, read-oriented access. Agent tokens may **read** (`GET /api/v1/servers`, diagnostics, config/registry reads, `GET /api/v1/index/search`) but may **not** perform mutating operations that touch server/security/config state. These return **`403 Forbidden`** with `operation requires admin access` for an agent token, mirroring the MCP `upstream_servers`/`quarantine_security` denylist so the two surfaces cannot drift. Socket (tray) connections authenticate as admin and are unaffected. Gated routes:
+  - **Servers** — add, remove, patch, enable, disable, restart, reconnect, quarantine, unquarantine, login, logout, config-to-secret, discover-tools, refresh, tool approve/block, the bulk `enable_all`/`disable_all`/`restart_all`, and the security scanner (`scan`, `scan/cancel`, `security/approve`, `security/reject`).
+  - **Config** — `POST /config/apply`, `PATCH /config`, `PATCH /config/docker-isolation` (config can add/remove/enable/disable servers). `POST /config/validate` is read-only and stays open.
+  - **Registries** — `POST/PUT/DELETE /registries[/{id}]` (source management) and `POST /registries/{id}/servers/{serverId}/add`. Registry browsing (`GET`) stays open.
+
 ## Base URL
 
 ```
@@ -556,6 +566,14 @@ enriched with approval state and 30-day usage. Read-only; consumers apply their
 search/filter/sort over the full set. For relevance-ranked discovery use
 `GET /api/v1/index/search` instead.
 
+> **Quarantine visibility:** `GET /api/v1/index/search` withholds the tools of
+> **quarantined** servers — their descriptions/schemas are the Tool Poisoning
+> Attack payload quarantine exists to contain, so the REST search path applies
+> the same server-level visibility gate as MCP `retrieve_tools`. Response shape
+> is unchanged (bare tool `name` + separate `server_name`); only which tools
+> appear is filtered. `GET /api/v1/tools` (above) is the unfiltered operator
+> overview and still lists quarantined servers' tools with their state.
+
 **Response:**
 ```json
 {

docs/features/agent-tokens.md

@@ -176,6 +176,18 @@ Server scoping is enforced at two levels:
 1. **Tool discovery** (`retrieve_tools`) — only returns tools from allowed servers
 2. **Tool execution** (`call_tool_*`) — blocks calls to out-of-scope servers
 
+## Administrative Operations Are Admin-Only
+
+Agent tokens can **discover and call** tools (within their scope and permission tier) but can **never administer servers**. Server-mutating operations require the admin API key (or a local tray/socket connection, which is admin by OS-level auth) on **every** surface — the MCP tools and the REST API share one policy (`internal/auth`), so an agent cannot do over HTTP what it is blocked from doing over MCP.
+
+Denied to agent tokens on both surfaces:
+
+- **Lifecycle**: add, remove, update/patch, enable, disable, restart, reconnect, refresh/discover-tools, add-from-registry, login/logout, move-config-value-to-secret
+- **Security state**: quarantine, unquarantine, tool approve/block, and the security scanner (scan start/cancel, security approve/reject)
+- **Config & registries**: applying/patching configuration (which can add/remove/enable/disable servers) and mutating registry sources — an agent must not bypass the per-server gate by rewriting config or a registry wholesale
+
+On the MCP surface (`upstream_servers`, `quarantine_security`) these return a tool error; on the REST surface (mutating `/api/v1/servers/...`, `/api/v1/config/...`, and `/api/v1/registries/...` routes) they return **`403 Forbidden`** (`operation requires admin access`). Read-only operations stay available to scoped tokens: `upstream_servers` `list`/`tail_log`, `GET /api/v1/servers`, per-server diagnostics, config/registry reads, and `GET /api/v1/index/search` (which honors quarantine — a quarantined server's tools are withheld from search on every surface).
+
 ## Profile Pinning
 
 A [profile](./profiles.md) scopes tool discovery and calls to a named subset of upstream servers. With `--profile-pin`, you can **bind a token to a single profile** so it can never operate outside it — regardless of the URL it connects to or any `set_profile` call it makes.

internal/auth/server_ops.go

@@ -0,0 +1,107 @@
+package auth
+
+// Server-mutating operation identifiers.
+//
+// These name the write operations on upstream MCP servers that are exposed on
+// BOTH the MCP `upstream_servers` / `quarantine_security` tools and the REST
+// `/api/v1/servers/...` surface. They are the single source of truth for which
+// operations agent tokens (non-admin) may NOT perform, so the two surfaces can
+// never drift apart again (issues #877/#878).
+//
+// The string values for the operations that also exist on the MCP
+// `upstream_servers` tool intentionally match that tool's operation names
+// (see internal/server/mcp.go: operationAdd/operationRemove and the literal
+// "update"/"patch"/"enable"/"disable"/"restart"/"refresh"/"add_from_registry"
+// dispatch), so the MCP denylist can consume this policy without changing its
+// observable behavior. REST-only operations (quarantine, login, …) use their
+// own identifiers; the MCP surface never passes those strings here, so listing
+// them is a no-op for MCP and closes the REST gap.
+const (
+	ServerOpAdd             = "add"
+	ServerOpRemove          = "remove"
+	ServerOpUpdate          = "update"
+	ServerOpPatch           = "patch"
+	ServerOpEnable          = "enable"
+	ServerOpDisable         = "disable"
+	ServerOpRestart         = "restart"
+	ServerOpRefresh         = "refresh"
+	ServerOpDiscoverTools   = "discover_tools"
+	ServerOpQuarantine      = "quarantine"
+	ServerOpUnquarantine    = "unquarantine"
+	ServerOpAddFromRegistry = "add_from_registry"
+	ServerOpLogin           = "login"
+	ServerOpLogout          = "logout"
+	ServerOpConfigToSecret  = "config_to_secret"
+	ServerOpApproveTools    = "approve_tools"
+	ServerOpBlockTools      = "block_tools"
+
+	// REST-only mutating operations that reach server/security/config state
+	// through routes OTHER than /api/v1/servers/{id}. They are denied to agents
+	// so the /servers gate cannot be bypassed via a sibling endpoint
+	// (config apply rewrites mcpServers; registry add creates a server; the
+	// security scanner mutates a server's approval state).
+	ServerOpConfigWrite     = "config_write"
+	ServerOpScan            = "scan"
+	ServerOpSecurityApprove = "security_approve"
+	ServerOpSecurityReject  = "security_reject"
+	ServerOpSecretWrite     = "secret_write"
+	ServerOpDiagnosticsFix  = "diagnostics_fix"
+)
+
+// agentDeniedServerOps is the canonical set of server/tool-mutating operations
+// that agent tokens are NOT permitted to perform, on any surface. Admin API
+// keys (and OS-authenticated socket connections, which authenticate as admin)
+// are never subject to this denylist.
+//
+// Read/observability operations (upstream_servers "list"/"tail_log", the
+// index search, GET diagnostics, …) are deliberately absent — they stay
+// available to scoped agent tokens.
+var agentDeniedServerOps = map[string]struct{}{
+	ServerOpAdd:             {},
+	ServerOpRemove:          {},
+	ServerOpUpdate:          {},
+	ServerOpPatch:           {},
+	ServerOpEnable:          {},
+	ServerOpDisable:         {},
+	ServerOpRestart:         {},
+	ServerOpRefresh:         {},
+	ServerOpDiscoverTools:   {},
+	ServerOpQuarantine:      {},
+	ServerOpUnquarantine:    {},
+	ServerOpAddFromRegistry: {},
+	ServerOpLogin:           {},
+	ServerOpLogout:          {},
+	ServerOpConfigToSecret:  {},
+	ServerOpApproveTools:    {},
+	ServerOpBlockTools:      {},
+	ServerOpConfigWrite:     {},
+	ServerOpScan:            {},
+	ServerOpSecurityApprove: {},
+	ServerOpSecurityReject:  {},
+	ServerOpSecretWrite:     {},
+	ServerOpDiagnosticsFix:  {},
+}
+
+// AgentDeniedServerOp reports whether the named operation is forbidden to agent
+// tokens (non-admin auth contexts). It is case-sensitive: callers pass the
+// canonical ServerOp* identifiers, never user-controlled free text.
+func AgentDeniedServerOp(op string) bool {
+	_, denied := agentDeniedServerOps[op]
+	return denied
+}
+
+// AuthorizeServerOp is the shared gate both surfaces use. It returns true when
+// the caller is permitted to perform op. An admin context (API key or
+// OS-authenticated socket) is always allowed. A nil context means the request
+// arrived without an AuthContext — the auth middleware only leaves it nil in
+// test/no-config passthrough scenarios that never occur once a real
+// *config.Config is loaded, so it is treated as allowed here (the middleware,
+// not this policy, is responsible for rejecting unauthenticated production
+// requests). Any present non-admin context (agent token) is denied when op is
+// on the denylist.
+func AuthorizeServerOp(ac *AuthContext, op string) bool {
+	if ac == nil || ac.IsAdmin() {
+		return true
+	}
+	return !AgentDeniedServerOp(op)
+}

internal/auth/server_ops_test.go

@@ -0,0 +1,57 @@
+package auth
+
+import "testing"
+
+func TestAgentDeniedServerOp(t *testing.T) {
+	denied := []string{
+		ServerOpAdd, ServerOpRemove, ServerOpUpdate, ServerOpPatch,
+		ServerOpEnable, ServerOpDisable, ServerOpRestart, ServerOpRefresh,
+		ServerOpDiscoverTools, ServerOpQuarantine, ServerOpUnquarantine,
+		ServerOpAddFromRegistry, ServerOpLogin, ServerOpLogout,
+		ServerOpConfigToSecret, ServerOpApproveTools, ServerOpBlockTools,
+		ServerOpConfigWrite, ServerOpScan, ServerOpSecurityApprove,
+		ServerOpSecurityReject, ServerOpSecretWrite, ServerOpDiagnosticsFix,
+	}
+	for _, op := range denied {
+		if !AgentDeniedServerOp(op) {
+			t.Errorf("expected op %q to be denied to agents", op)
+		}
+	}
+
+	// Read/observability ops that agents keep.
+	allowed := []string{"list", "tail_log", "inspect", "", "LIST", "Enable"}
+	for _, op := range allowed {
+		if AgentDeniedServerOp(op) {
+			t.Errorf("expected op %q to be allowed for agents", op)
+		}
+	}
+}
+
+func TestAuthorizeServerOp(t *testing.T) {
+	agent := &AuthContext{Type: AuthTypeAgent, AgentName: "a"}
+	admin := AdminContext()
+	adminUser := &AuthContext{Type: AuthTypeAdminUser}
+	user := &AuthContext{Type: AuthTypeUser}
+
+	tests := []struct {
+		name string
+		ac   *AuthContext
+		op   string
+		want bool
+	}{
+		{"nil passthrough (test/no-config)", nil, ServerOpEnable, true},
+		{"admin allowed on denied op", admin, ServerOpRemove, true},
+		{"admin_user allowed on denied op", adminUser, ServerOpQuarantine, true},
+		{"agent denied on denied op", agent, ServerOpEnable, false},
+		{"agent denied on quarantine", agent, ServerOpQuarantine, false},
+		{"agent allowed on read op", agent, "list", true},
+		{"non-admin user denied on denied op", user, ServerOpRestart, false},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := AuthorizeServerOp(tc.ac, tc.op); got != tc.want {
+				t.Errorf("AuthorizeServerOp(%+v, %q) = %v, want %v", tc.ac, tc.op, got, tc.want)
+			}
+		})
+	}
+}

internal/httpapi/agent_token_gating_test.go

@@ -0,0 +1,136 @@
+package httpapi
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+// adminConfigController wraps a ServerController so the auth middleware sees a
+// real *config.Config (required to distinguish admin from agent tokens) with a
+// known admin key.
+type adminConfigController struct {
+	ServerController
+	apiKey string
+}
+
+func (c *adminConfigController) GetCurrentConfig() interface{} {
+	return &config.Config{APIKey: c.apiKey}
+}
+
+// gateErrMsg is the exact body the requireServerOp gate writes. Both the agent
+// (blocked) and admin (allowed) assertions key off it so the test verifies the
+// gate specifically, independent of whatever the next layer returns.
+const gateErrMsg = "operation requires admin access"
+
+// mutatingServerRoutes enumerates every server-mutating REST route that must be
+// denied to agent tokens (issues #877/#878). Adding a new mutating /servers
+// route without gating it should make this table incomplete — keep it in sync.
+var mutatingServerRoutes = []struct {
+	name   string
+	method string
+	path   string
+}{
+	{"add", http.MethodPost, "/api/v1/servers"},
+	{"import", http.MethodPost, "/api/v1/servers/import"},
+	{"import-json", http.MethodPost, "/api/v1/servers/import/json"},
+	{"import-path", http.MethodPost, "/api/v1/servers/import/path"},
+	{"reconnect", http.MethodPost, "/api/v1/servers/reconnect"},
+	{"restart-all", http.MethodPost, "/api/v1/servers/restart_all"},
+	{"enable-all", http.MethodPost, "/api/v1/servers/enable_all"},
+	{"disable-all", http.MethodPost, "/api/v1/servers/disable_all"},
+	{"patch", http.MethodPatch, "/api/v1/servers/test-server"},
+	{"remove", http.MethodDelete, "/api/v1/servers/test-server"},
+	{"config-to-secret", http.MethodPost, "/api/v1/servers/test-server/config-to-secret"},
+	{"enable", http.MethodPost, "/api/v1/servers/test-server/enable"},
+	{"disable", http.MethodPost, "/api/v1/servers/test-server/disable"},
+	{"restart", http.MethodPost, "/api/v1/servers/test-server/restart"},
+	{"login", http.MethodPost, "/api/v1/servers/test-server/login"},
+	{"logout", http.MethodPost, "/api/v1/servers/test-server/logout"},
+	{"quarantine", http.MethodPost, "/api/v1/servers/test-server/quarantine"},
+	{"unquarantine", http.MethodPost, "/api/v1/servers/test-server/unquarantine"},
+	{"discover-tools", http.MethodPost, "/api/v1/servers/test-server/discover-tools"},
+	{"refresh", http.MethodPost, "/api/v1/servers/test-server/refresh"},
+	{"tools-approve", http.MethodPost, "/api/v1/servers/test-server/tools/approve"},
+	{"tools-block", http.MethodPost, "/api/v1/servers/test-server/tools/block"},
+	{"scan", http.MethodPost, "/api/v1/servers/test-server/scan"},
+	{"scan-cancel", http.MethodPost, "/api/v1/servers/test-server/scan/cancel"},
+	{"security-approve", http.MethodPost, "/api/v1/servers/test-server/security/approve"},
+	{"security-reject", http.MethodPost, "/api/v1/servers/test-server/security/reject"},
+	// Sibling routes that mutate server/registry state OUTSIDE /servers/{id} —
+	// an agent must not bypass the /servers gate through them (issue #878).
+	{"config-apply", http.MethodPost, "/api/v1/config/apply"},
+	{"config-patch", http.MethodPatch, "/api/v1/config"},
+	{"config-docker-isolation", http.MethodPatch, "/api/v1/config/docker-isolation"},
+	{"registry-add-source", http.MethodPost, "/api/v1/registries"},
+	{"registry-edit-source", http.MethodPut, "/api/v1/registries/reg1"},
+	{"registry-remove-source", http.MethodDelete, "/api/v1/registries/reg1"},
+	{"registry-add-server", http.MethodPost, "/api/v1/registries/reg1/servers/srv1/add"},
+	{"secret-set", http.MethodPost, "/api/v1/secrets"},
+	{"secret-delete", http.MethodDelete, "/api/v1/secrets/tok"},
+	{"secret-migrate", http.MethodPost, "/api/v1/secrets/migrate"},
+	{"diagnostics-fix", http.MethodPost, "/api/v1/diagnostics/fix"},
+	{"scanner-enable", http.MethodPost, "/api/v1/security/scanners/s1/enable"},
+	{"scanner-disable", http.MethodPost, "/api/v1/security/scanners/s1/disable"},
+	{"scanner-config", http.MethodPut, "/api/v1/security/scanners/s1/config"},
+	{"scanner-install", http.MethodPost, "/api/v1/security/scanners/install"},
+	{"scanner-remove", http.MethodDelete, "/api/v1/security/scanners/s1"},
+	{"scan-all", http.MethodPost, "/api/v1/security/scan-all"},
+	{"cancel-all", http.MethodPost, "/api/v1/security/cancel-all"},
+	{"connect-client", http.MethodPost, "/api/v1/connect/claude"},
+	{"connect-undo", http.MethodPost, "/api/v1/connect/claude/undo"},
+	{"connect-disconnect", http.MethodDelete, "/api/v1/connect/claude"},
+}
+
+// TestMutatingServerRoutes_AgentTokenForbidden asserts every mutating server
+// route rejects an agent token with 403 (issues #877/#878). The agent is scoped
+// to all servers with read+write permission, so the denial is the operation
+// policy — not a scope/permission miss.
+func TestMutatingServerRoutes_AgentTokenForbidden(t *testing.T) {
+	for _, rt := range mutatingServerRoutes {
+		t.Run(rt.name, func(t *testing.T) {
+			ctrl := &adminConfigController{ServerController: &MockServerController{}, apiKey: "admin-secret"}
+			srv, agentToken := agentTokenServer(t, ctrl)
+
+			req := httptest.NewRequest(rt.method, rt.path, nil)
+			req.Header.Set("X-API-Key", agentToken)
+			w := httptest.NewRecorder()
+
+			srv.ServeHTTP(w, req)
+
+			assert.Equal(t, http.StatusForbidden, w.Code, "agent token must be forbidden")
+			assert.Contains(t, w.Body.String(), "admin access")
+		})
+	}
+}
+
+// TestMutatingServerRoutes_AdminAllowed asserts a full API key is never blocked
+// by the agent-token gate — it reaches the next layer (whatever status that
+// returns), proving back-compat for admin callers (issues #877/#878).
+func TestMutatingServerRoutes_AdminAllowed(t *testing.T) {
+	const adminKey = "admin-secret"
+	for _, rt := range mutatingServerRoutes {
+		t.Run(rt.name, func(t *testing.T) {
+			ctrl := &adminConfigController{ServerController: &MockServerController{}, apiKey: adminKey}
+			logger := zap.NewNop().Sugar()
+			srv := NewServer(ctrl, logger, nil)
+
+			req := httptest.NewRequest(rt.method, rt.path, nil)
+			req.Header.Set("X-API-Key", adminKey)
+			w := httptest.NewRecorder()
+
+			srv.ServeHTTP(w, req)
+
+			require.NotEqual(t, http.StatusUnauthorized, w.Code, "admin key must authenticate")
+			// The gate must not fire for admin: its exact message must be absent.
+			assert.NotContains(t, w.Body.String(), gateErrMsg,
+				"admin request must pass the agent-token gate (route %s)", rt.path)
+		})
+	}
+}