Skip to content

Commit 0f1a02b

Browse files
feat(quarantine): atomic Block (approve+disable) endpoint for tools (MCP-2198) (#653)
* feat(quarantine): atomic Block (approve+disable) endpoint for tools (MCP-2198) Add an atomic "block" = approve+disable for quarantined tools, performed server-side as a single all-or-nothing write per tool so a tool is never observably left in the approved+enabled state. - internal/runtime/tool_quarantine.go: BlockTools / BlockAllTools — promote status to approved (ReasonUserApprove) AND set Disabled in one SaveToolApproval; emit tool_blocked activity + tools_blocked SSE. Block only ever disables, so config-denied tools need no special handling. - internal/httpapi/server.go: POST /api/v1/servers/{id}/tools/block (handleBlockTools), mirrors handleApproveTools — {tools:[...]} or {block_all:true}; 400 if neither. - internal/server/mcp.go: block_tool / block_all_tools ops on quarantine_security. - internal/server/server.go: controller passthroughs. - Tests: httpapi + runtime (end state approved && Disabled; specific/all/empty). Backend lane of GH #632 (parent MCP-2196). Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(quarantine): single-line OAS block description + MCP block_tool tests Addresses CodexReviewer request_changes on PR #653: 1. The /servers/{id}/tools/block @description swag annotation was split across multiple comment lines, so swag truncated the generated oas/swagger.yaml description at the first line. Collapse it to a single line and regenerate (make swagger) so the full sentence is emitted. 2. Add internal/server coverage for the new quarantine_security MCP ops block_tool / block_all_tools: dispatch routing, required-arg errors (missing name / tool_name), and the atomic approve+disable success path (RV-3). A runtime-backed proxy helper shares storage with the runtime so the handlers' runtime.BlockTools path executes for real. Related #653 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
1 parent 8f39779 commit 0f1a02b

14 files changed

Lines changed: 771 additions & 8 deletions

CLAUDE.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -368,12 +368,12 @@ See [docs/configuration.md](docs/configuration.md) for complete reference.
368368
- **`call_tool_write`** - Proxy write tool calls to upstream servers (Spec 018)
369369
- **`call_tool_destructive`** - Proxy destructive tool calls to upstream servers (Spec 018)
370370
- **`code_execution`** - Execute JavaScript to orchestrate multiple tools (disabled by default)
371-
- **`upstream_servers`** - CRUD operations for server management. Spec 049: list/get entries carry a conditional `tools` count block only when a server has ≥1 non-callable tool.
372-
- **`quarantine_security`** - Security quarantine management: list/inspect quarantined servers, inspect/approve/approve-all tools (Spec 032)
371+
- **`upstream_servers`** - CRUD for server management (Spec 049).
372+
- **`quarantine_security`** - Security quarantine: list/inspect quarantined servers, inspect/approve/approve-all tools, and block/block-all (atomic approve+disable) (Spec 032).
373373
374374
**Tool Format**: `<serverName>:<toolName>` (e.g., `github:create_issue`)
375375
376-
**Intent Declaration (Spec 018)**: Tool variants enable granular IDE permission control. The `operation_type` is automatically inferred from the tool variant (`call_tool_read` → "read", etc.). Optional `intent` fields for audit:
376+
**Intent Declaration (Spec 018)**: Tool variants enable granular IDE permission control; `operation_type` is inferred from the variant (`call_tool_read` → "read"). Optional `intent` fields for audit:
377377
```json
378378
{
379379
"intent": {
@@ -406,6 +406,7 @@ See [docs/configuration.md](docs/configuration.md) for complete reference.
406406
| `DELETE /api/v1/tokens/{name}` | Revoke agent token |
407407
| `POST /api/v1/tokens/{name}/regenerate` | Regenerate agent token secret |
408408
| `POST /api/v1/servers/{id}/tools/approve` | Approve pending/changed tools (Spec 032) |
409+
| `POST /api/v1/servers/{id}/tools/block` | Block (approve+disable) tools (Spec 032) |
409410
| `GET /api/v1/servers/{id}/tools/{tool}/diff` | View tool description/schema changes (Spec 032) |
410411
| `GET /api/v1/servers/{id}/tools/export` | Export tool approval records (Spec 032) |
411412
| `POST /api/v1/feedback` | Submit feedback/bug report (proxied to GitHub Issues) |

docs/api/rest-api.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,42 @@ Or approve all pending/changed tools:
437437
}
438438
```
439439

440+
#### POST /api/v1/servers/{name}/tools/block
441+
442+
Atomically **block** tools = approve **and** disable them in a single server-side
443+
operation. Use this to acknowledge a pending/changed tool (clearing its
444+
quarantine flag) while keeping it hidden from MCP clients. The approve and
445+
disable land in one write per tool, so a tool is never left in the
446+
approved+enabled state.
447+
448+
**Request Body:**
449+
```json
450+
{
451+
"tools": ["create_issue", "delete_repo"]
452+
}
453+
```
454+
455+
Or block all pending/changed tools:
456+
```json
457+
{
458+
"block_all": true
459+
}
460+
```
461+
462+
**Response:**
463+
```json
464+
{
465+
"success": true,
466+
"data": {
467+
"blocked": 2,
468+
"tools": ["create_issue", "delete_repo"],
469+
"message": "Blocked 2 tools for server github-server"
470+
}
471+
}
472+
```
473+
474+
Returns `400` if neither `tools` nor `block_all` is provided.
475+
440476
#### GET /api/v1/servers/{name}/tools/{tool}/diff
441477

442478
Get the description/schema diff for a changed tool. The response exposes every

docs/features/security-quarantine.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,23 @@ See [Tool Quarantine](./tool-quarantine.md) for complete documentation on:
178178
- Configuration: `quarantine_enabled` and `skip_quarantine`
179179
- REST API endpoints for tool approval management
180180

181+
### Block (approve + disable)
182+
183+
When reviewing a pending or changed tool you may want to **acknowledge it but
184+
keep it hidden** from MCP clients — for example, dismissing a noisy "changed"
185+
flag for a tool you never intend to use. The **block** operation does this
186+
atomically: it approves the tool (clearing the quarantine flag) **and** disables
187+
it in a single, all-or-nothing server-side write, so a tool is never left in the
188+
approved+enabled state.
189+
190+
- **REST**: `POST /api/v1/servers/{id}/tools/block` with `{"tools":[...]}` or
191+
`{"block_all": true}`.
192+
- **MCP**: `quarantine_security` operations `block_tool` (with `name` +
193+
`tool_name`) and `block_all_tools` (with `name`).
194+
195+
A blocked tool can be re-exposed later with the normal enable operation
196+
(`POST /api/v1/servers/{id}/tools/{tool}/enabled` with `{"enabled": true}`).
197+
181198
## Disabling Quarantine
182199

183200
**Not recommended**, but you can opt out of quarantine globally by setting a

internal/httpapi/contracts_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,10 @@ func (m *MockServerController) ListToolApprovals(_ string) ([]*storage.ToolAppro
360360
}
361361
func (m *MockServerController) ApproveTools(_ string, _ []string, _ string) error { return nil }
362362
func (m *MockServerController) ApproveAllTools(_ string, _ string) (int, error) { return 0, nil }
363+
func (m *MockServerController) BlockTools(_ string, _ []string, _ string) (int, error) {
364+
return 0, nil
365+
}
366+
func (m *MockServerController) BlockAllTools(_ string, _ string) (int, error) { return 0, nil }
363367
func (m *MockServerController) GetToolApproval(_, _ string) (*storage.ToolApprovalRecord, error) {
364368
return nil, nil
365369
}

internal/httpapi/security_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,10 @@ func (m *baseController) ListToolApprovals(_ string) ([]*storage.ToolApprovalRec
352352
}
353353
func (m *baseController) ApproveTools(_ string, _ []string, _ string) error { return nil }
354354
func (m *baseController) ApproveAllTools(_ string, _ string) (int, error) { return 0, nil }
355+
func (m *baseController) BlockTools(_ string, _ []string, _ string) (int, error) {
356+
return 0, nil
357+
}
358+
func (m *baseController) BlockAllTools(_ string, _ string) (int, error) { return 0, nil }
355359
func (m *baseController) GetToolApproval(_, _ string) (*storage.ToolApprovalRecord, error) {
356360
return nil, nil
357361
}

internal/httpapi/server.go

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,10 @@ type ServerController interface {
169169
ListToolApprovals(serverName string) ([]*storage.ToolApprovalRecord, error)
170170
ApproveTools(serverName string, toolNames []string, approvedBy string) error
171171
ApproveAllTools(serverName string, approvedBy string) (int, error)
172+
// BlockTools / BlockAllTools atomically approve+disable tools (MCP-2198):
173+
// all-or-nothing so a tool is never left approved+enabled.
174+
BlockTools(serverName string, toolNames []string, blockedBy string) (int, error)
175+
BlockAllTools(serverName string, blockedBy string) (int, error)
172176
GetToolApproval(serverName, toolName string) (*storage.ToolApprovalRecord, error)
173177

174178
// Onboarding wizard (Spec 046)
@@ -634,6 +638,9 @@ func (s *Server) setupRoutes() {
634638

635639
// Tool-level quarantine (Spec 032)
636640
r.Post("/tools/approve", s.handleApproveTools)
641+
// Atomic block = approve+disable (MCP-2198). Server-side so the
642+
// pair is all-or-nothing — a tool is never left approved+enabled.
643+
r.Post("/tools/block", s.handleBlockTools)
637644
r.Post("/tools/{tool}/enabled", s.handleSetToolEnabled)
638645
// Bulk per-tool enable/disable. Mirrors /servers/enable_all
639646
// + /servers/disable_all but scoped to a single server's tools.
@@ -4665,6 +4672,70 @@ func (s *Server) handleApproveTools(w http.ResponseWriter, r *http.Request) {
46654672
})
46664673
}
46674674

4675+
// handleBlockTools handles POST /api/v1/servers/{id}/tools/block
4676+
// A block is an atomic approve+disable performed in the runtime so the pair is
4677+
// all-or-nothing — a tool is never left approved+enabled. Mirrors
4678+
// handleApproveTools: accepts either {"tools":[...]} or {"block_all":true}.
4679+
//
4680+
// @Summary Block (approve+disable) tools for a server
4681+
// @Description Atomically approves AND disables the given tools (or all pending/changed tools when block_all=true) for a server. The approve and disable land in a single write per tool, so a tool is never left in the approved+enabled state. The "blocked" field counts tools actually blocked.
4682+
// @Tags servers
4683+
// @Accept json
4684+
// @Produce json
4685+
// @Security ApiKeyAuth
4686+
// @Security ApiKeyQuery
4687+
// @Param id path string true "Server ID or name"
4688+
// @Success 200 {object} contracts.SuccessResponse "Block result"
4689+
// @Failure 400 {object} contracts.ErrorResponse "Bad request"
4690+
// @Failure 500 {object} contracts.ErrorResponse "Internal server error"
4691+
// @Router /api/v1/servers/{id}/tools/block [post]
4692+
func (s *Server) handleBlockTools(w http.ResponseWriter, r *http.Request) {
4693+
serverID := chi.URLParam(r, "id")
4694+
if serverID == "" {
4695+
s.writeError(w, r, http.StatusBadRequest, "Server ID required")
4696+
return
4697+
}
4698+
4699+
var req struct {
4700+
Tools []string `json:"tools"`
4701+
BlockAll bool `json:"block_all"`
4702+
}
4703+
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
4704+
s.writeError(w, r, http.StatusBadRequest, fmt.Sprintf("Invalid request body: %v", err))
4705+
return
4706+
}
4707+
4708+
if req.BlockAll {
4709+
count, err := s.controller.BlockAllTools(serverID, "api")
4710+
if err != nil {
4711+
s.writeError(w, r, http.StatusInternalServerError, fmt.Sprintf("Failed to block tools: %v", err))
4712+
return
4713+
}
4714+
s.writeSuccess(w, map[string]interface{}{
4715+
"blocked": count,
4716+
"message": fmt.Sprintf("Blocked %d tools for server %s", count, serverID),
4717+
})
4718+
return
4719+
}
4720+
4721+
if len(req.Tools) == 0 {
4722+
s.writeError(w, r, http.StatusBadRequest, "Either 'tools' array or 'block_all: true' required")
4723+
return
4724+
}
4725+
4726+
count, err := s.controller.BlockTools(serverID, req.Tools, "api")
4727+
if err != nil {
4728+
s.writeError(w, r, http.StatusInternalServerError, fmt.Sprintf("Failed to block tools: %v", err))
4729+
return
4730+
}
4731+
4732+
s.writeSuccess(w, map[string]interface{}{
4733+
"blocked": count,
4734+
"tools": req.Tools,
4735+
"message": fmt.Sprintf("Blocked %d tools for server %s", count, serverID),
4736+
})
4737+
}
4738+
46684739
// handleGetToolDiff handles GET /api/v1/servers/{id}/tools/{tool}/diff
46694740
func (s *Server) handleSetToolEnabled(w http.ResponseWriter, r *http.Request) {
46704741
authCtx := auth.AuthContextFromContext(r.Context())

internal/httpapi/tool_quarantine_test.go

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,11 @@ type mockToolQuarantineController struct {
3939
setAllToolsEnabledTo *bool
4040
setAllToolsEnabledFor string
4141
setAllToolsChangedFake int
42+
blockErr error
43+
blockAllErr error
44+
blockedCount int
45+
blockedTools []string
46+
blockedServer string
4247
}
4348

4449
func (m *mockToolQuarantineController) GetCurrentConfig() any {
@@ -68,6 +73,20 @@ func (m *mockToolQuarantineController) ApproveAllTools(serverName string, approv
6873
return m.approvedCount, m.approveAllErr
6974
}
7075

76+
func (m *mockToolQuarantineController) BlockTools(serverName string, toolNames []string, _ string) (int, error) {
77+
m.blockedServer = serverName
78+
m.blockedTools = toolNames
79+
if m.blockErr != nil {
80+
return 0, m.blockErr
81+
}
82+
return len(toolNames), nil
83+
}
84+
85+
func (m *mockToolQuarantineController) BlockAllTools(serverName string, _ string) (int, error) {
86+
m.blockedServer = serverName
87+
return m.blockedCount, m.blockAllErr
88+
}
89+
7190
func (m *mockToolQuarantineController) GetToolApproval(serverName, toolName string) (*storage.ToolApprovalRecord, error) {
7291
for _, a := range m.approvals {
7392
if a.ServerName == serverName && a.ToolName == toolName {
@@ -338,6 +357,106 @@ func TestHandleApproveTools_ApproveError(t *testing.T) {
338357
assert.Equal(t, http.StatusInternalServerError, w.Code)
339358
}
340359

360+
// =============================================================================
361+
// MCP-2198: atomic block (approve+disable) handler tests
362+
// =============================================================================
363+
364+
func TestHandleBlockTools_SpecificTools(t *testing.T) {
365+
ctrl := &mockToolQuarantineController{apiKey: "test-key"}
366+
logger := zap.NewNop().Sugar()
367+
server := NewServer(ctrl, logger, nil)
368+
369+
body := `{"tools": ["create_issue", "list_repos"]}`
370+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/block", bytes.NewBufferString(body))
371+
req.Header.Set("X-API-Key", "test-key")
372+
w := httptest.NewRecorder()
373+
374+
server.ServeHTTP(w, req)
375+
376+
assert.Equal(t, http.StatusOK, w.Code)
377+
assert.Equal(t, "github", ctrl.blockedServer)
378+
assert.Equal(t, []string{"create_issue", "list_repos"}, ctrl.blockedTools)
379+
380+
var resp map[string]interface{}
381+
err := json.Unmarshal(w.Body.Bytes(), &resp)
382+
require.NoError(t, err)
383+
data := resp["data"].(map[string]interface{})
384+
assert.Equal(t, float64(2), data["blocked"])
385+
}
386+
387+
func TestHandleBlockTools_BlockAll(t *testing.T) {
388+
ctrl := &mockToolQuarantineController{
389+
apiKey: "test-key",
390+
blockedCount: 5,
391+
}
392+
logger := zap.NewNop().Sugar()
393+
server := NewServer(ctrl, logger, nil)
394+
395+
body := `{"block_all": true}`
396+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/block", bytes.NewBufferString(body))
397+
req.Header.Set("X-API-Key", "test-key")
398+
w := httptest.NewRecorder()
399+
400+
server.ServeHTTP(w, req)
401+
402+
assert.Equal(t, http.StatusOK, w.Code)
403+
assert.Equal(t, "github", ctrl.blockedServer)
404+
405+
var resp map[string]interface{}
406+
err := json.Unmarshal(w.Body.Bytes(), &resp)
407+
require.NoError(t, err)
408+
data := resp["data"].(map[string]interface{})
409+
assert.Equal(t, float64(5), data["blocked"])
410+
}
411+
412+
func TestHandleBlockTools_EmptyToolsAndNoBlockAll(t *testing.T) {
413+
ctrl := &mockToolQuarantineController{apiKey: "test-key"}
414+
logger := zap.NewNop().Sugar()
415+
server := NewServer(ctrl, logger, nil)
416+
417+
body := `{"tools": []}`
418+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/block", bytes.NewBufferString(body))
419+
req.Header.Set("X-API-Key", "test-key")
420+
w := httptest.NewRecorder()
421+
422+
server.ServeHTTP(w, req)
423+
424+
assert.Equal(t, http.StatusBadRequest, w.Code)
425+
}
426+
427+
func TestHandleBlockTools_InvalidJSON(t *testing.T) {
428+
ctrl := &mockToolQuarantineController{apiKey: "test-key"}
429+
logger := zap.NewNop().Sugar()
430+
server := NewServer(ctrl, logger, nil)
431+
432+
body := `{invalid`
433+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/block", bytes.NewBufferString(body))
434+
req.Header.Set("X-API-Key", "test-key")
435+
w := httptest.NewRecorder()
436+
437+
server.ServeHTTP(w, req)
438+
439+
assert.Equal(t, http.StatusBadRequest, w.Code)
440+
}
441+
442+
func TestHandleBlockTools_BlockError(t *testing.T) {
443+
ctrl := &mockToolQuarantineController{
444+
apiKey: "test-key",
445+
blockErr: fmt.Errorf("server not found"),
446+
}
447+
logger := zap.NewNop().Sugar()
448+
server := NewServer(ctrl, logger, nil)
449+
450+
body := `{"tools": ["create_issue"]}`
451+
req := httptest.NewRequest("POST", "/api/v1/servers/github/tools/block", bytes.NewBufferString(body))
452+
req.Header.Set("X-API-Key", "test-key")
453+
w := httptest.NewRecorder()
454+
455+
server.ServeHTTP(w, req)
456+
457+
assert.Equal(t, http.StatusInternalServerError, w.Code)
458+
}
459+
341460
func TestHandleGetToolDiff_ChangedTool(t *testing.T) {
342461
ctrl := &mockToolQuarantineController{
343462
apiKey: "test-key",

0 commit comments

Comments
 (0)