Skip to content

Commit ba2dcad

Browse files
electrolobzikclaude
andcommitted
fix(security): admin gate on tool-toggle REST endpoints
The MCP-layer quarantine_security tool gates enable_tool / disable_tool behind authCtx.IsAdmin() (mcp.go:2326). The three new REST endpoints that landed in this PR did NOT mirror that check: POST /api/v1/servers/{id}/tools/{tool}/enabled POST /api/v1/servers/{id}/tools/enable_all POST /api/v1/servers/{id}/tools/disable_all Result: an agent token (mcp_agt_ prefix) — scoped to a subset of servers and read-only permissions via the agent-token system — could hit any of these REST endpoints and flip per-tool visibility on any server, bypassing the admin gate the MCP layer enforces. Add the guard at the top of handleSetToolEnabled and the handleSetAllToolsEnabled closure (covers both enable_all and disable_all routes). Non-admin or unauthenticated callers receive 403 "operation requires admin access". Scope limited to the new endpoints. The pre-existing per-server REST mutation endpoints (e.g. POST /api/v1/servers/{name}/enable, .../quarantine, .../tools/approve) also lack admin checks — that's a broader hardening pattern worth a separate PR rather than entangling it with the per-tool feature. Regression tests in tool_quarantine_test.go: - TestHandleSetToolEnabled_AgentTokenForbidden - TestHandleSetAllToolsEnabled_EnableAll_AgentTokenForbidden - TestHandleSetAllToolsEnabled_DisableAll_AgentTokenForbidden - TestHandleSetToolEnabled_AdminKeyAllowed (positive control) Helpers: mockToolToggleController wraps the existing mock with a real *config.Config so the auth middleware activates; agentTokenServer mints a live agent token via the token store. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c3e99ac commit ba2dcad

2 files changed

Lines changed: 153 additions & 0 deletions

File tree

internal/httpapi/server.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3777,6 +3777,12 @@ func (s *Server) handleApproveTools(w http.ResponseWriter, r *http.Request) {
37773777

37783778
// handleGetToolDiff handles GET /api/v1/servers/{id}/tools/{tool}/diff
37793779
func (s *Server) handleSetToolEnabled(w http.ResponseWriter, r *http.Request) {
3780+
authCtx := auth.AuthContextFromContext(r.Context())
3781+
if authCtx == nil || !authCtx.IsAdmin() {
3782+
s.writeError(w, r, http.StatusForbidden, "operation requires admin access")
3783+
return
3784+
}
3785+
37803786
serverID := chi.URLParam(r, "id")
37813787
toolName := chi.URLParam(r, "tool")
37823788
if serverID == "" || toolName == "" {
@@ -3837,6 +3843,12 @@ func (s *Server) handleSetToolEnabled(w http.ResponseWriter, r *http.Request) {
38373843
// @Router /api/v1/servers/{id}/tools/disable_all [post]
38383844
func (s *Server) handleSetAllToolsEnabled(enabled bool) http.HandlerFunc {
38393845
return func(w http.ResponseWriter, r *http.Request) {
3846+
authCtx := auth.AuthContextFromContext(r.Context())
3847+
if authCtx == nil || !authCtx.IsAdmin() {
3848+
s.writeError(w, r, http.StatusForbidden, "operation requires admin access")
3849+
return
3850+
}
3851+
38403852
serverID := chi.URLParam(r, "id")
38413853
if serverID == "" {
38423854
s.writeError(w, r, http.StatusBadRequest, "Server ID required")

internal/httpapi/tool_quarantine_test.go

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@ import (
77
"net/http"
88
"net/http/httptest"
99
"testing"
10+
"time"
1011

1112
"github.com/stretchr/testify/assert"
1213
"github.com/stretchr/testify/require"
1314
"go.uber.org/zap"
1415

16+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/auth"
1517
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
1618
"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
1719
)
@@ -592,3 +594,142 @@ func TestHandleUnquarantineServer_Error(t *testing.T) {
592594

593595
assert.Equal(t, http.StatusInternalServerError, w.Code)
594596
}
597+
598+
// =============================================================================
599+
// Security: Admin gate on tool-toggle REST endpoints (fix: agent bypass)
600+
// =============================================================================
601+
602+
// agentTokenServer returns a Server wired with a fake agent token so that
603+
// requests carrying that raw token go through the middleware as AuthTypeAgent
604+
// (non-admin). The returned rawToken must be placed in X-API-Key.
605+
func agentTokenServer(t *testing.T, ctrl ServerController) (*Server, string) {
606+
t.Helper()
607+
tmpDir := t.TempDir()
608+
_, err := auth.GetOrCreateHMACKey(tmpDir)
609+
require.NoError(t, err)
610+
611+
rawToken, err := auth.GenerateToken()
612+
require.NoError(t, err)
613+
614+
agentToken := &auth.AgentToken{
615+
Name: "test-agent",
616+
TokenPrefix: auth.TokenPrefix(rawToken),
617+
AllowedServers: []string{"*"},
618+
Permissions: []string{auth.PermRead, auth.PermWrite},
619+
ExpiresAt: time.Now().Add(24 * time.Hour),
620+
CreatedAt: time.Now(),
621+
}
622+
623+
store := &testTokenStore{
624+
validateFunc: func(token string, _ []byte) (*auth.AgentToken, error) {
625+
if token == rawToken {
626+
return agentToken, nil
627+
}
628+
return nil, fmt.Errorf("token not found")
629+
},
630+
}
631+
632+
logger := zap.NewNop().Sugar()
633+
srv := NewServer(ctrl, logger, nil)
634+
srv.SetTokenStore(store, tmpDir)
635+
return srv, rawToken
636+
}
637+
638+
// mockToolToggleController embeds mockToolQuarantineController and overrides
639+
// GetCurrentConfig to return a real *config.Config so the auth middleware
640+
// enforces authentication (required to distinguish admin vs agent).
641+
type mockToolToggleController struct {
642+
mockToolQuarantineController
643+
}
644+
645+
func (m *mockToolToggleController) GetCurrentConfig() any {
646+
return &config.Config{APIKey: m.apiKey}
647+
}
648+
649+
func TestHandleSetToolEnabled_AgentTokenForbidden(t *testing.T) {
650+
ctrl := &mockToolToggleController{
651+
mockToolQuarantineController: mockToolQuarantineController{
652+
apiKey: "admin-secret",
653+
approvals: []*storage.ToolApprovalRecord{{
654+
ServerName: "github",
655+
ToolName: "create_issue",
656+
Status: storage.ToolApprovalStatusApproved,
657+
}},
658+
},
659+
}
660+
661+
srv, agentToken := agentTokenServer(t, ctrl)
662+
663+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/create_issue/enabled",
664+
bytes.NewBufferString(`{"enabled":false}`))
665+
req.Header.Set("X-API-Key", agentToken)
666+
w := httptest.NewRecorder()
667+
668+
srv.ServeHTTP(w, req)
669+
670+
assert.Equal(t, http.StatusForbidden, w.Code)
671+
assert.Contains(t, w.Body.String(), "admin access")
672+
}
673+
674+
func TestHandleSetAllToolsEnabled_EnableAll_AgentTokenForbidden(t *testing.T) {
675+
ctrl := &mockToolToggleController{
676+
mockToolQuarantineController: mockToolQuarantineController{
677+
apiKey: "admin-secret",
678+
},
679+
}
680+
681+
srv, agentToken := agentTokenServer(t, ctrl)
682+
683+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/enable_all", nil)
684+
req.Header.Set("X-API-Key", agentToken)
685+
w := httptest.NewRecorder()
686+
687+
srv.ServeHTTP(w, req)
688+
689+
assert.Equal(t, http.StatusForbidden, w.Code)
690+
assert.Contains(t, w.Body.String(), "admin access")
691+
}
692+
693+
func TestHandleSetAllToolsEnabled_DisableAll_AgentTokenForbidden(t *testing.T) {
694+
ctrl := &mockToolToggleController{
695+
mockToolQuarantineController: mockToolQuarantineController{
696+
apiKey: "admin-secret",
697+
},
698+
}
699+
700+
srv, agentToken := agentTokenServer(t, ctrl)
701+
702+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/disable_all", nil)
703+
req.Header.Set("X-API-Key", agentToken)
704+
w := httptest.NewRecorder()
705+
706+
srv.ServeHTTP(w, req)
707+
708+
assert.Equal(t, http.StatusForbidden, w.Code)
709+
assert.Contains(t, w.Body.String(), "admin access")
710+
}
711+
712+
func TestHandleSetToolEnabled_AdminKeyAllowed(t *testing.T) {
713+
ctrl := &mockToolToggleController{
714+
mockToolQuarantineController: mockToolQuarantineController{
715+
apiKey: "admin-secret",
716+
approvals: []*storage.ToolApprovalRecord{{
717+
ServerName: "github",
718+
ToolName: "create_issue",
719+
Status: storage.ToolApprovalStatusApproved,
720+
}},
721+
},
722+
}
723+
724+
logger := zap.NewNop().Sugar()
725+
srv := NewServer(ctrl, logger, nil)
726+
727+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/create_issue/enabled",
728+
bytes.NewBufferString(`{"enabled":false}`))
729+
req.Header.Set("X-API-Key", "admin-secret")
730+
w := httptest.NewRecorder()
731+
732+
srv.ServeHTTP(w, req)
733+
734+
assert.Equal(t, http.StatusOK, w.Code)
735+
}

0 commit comments

Comments
 (0)