From cbf929fe2dfe2304ac9993a4e5bd14e7a52a9722 Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 9 Sep 2026 07:46:45 +0000 Subject: [PATCH] fix: audit refused and failed mcp write calls, scope key assignment to token Every early return from a write-tool handler skipped both the tool's own LogCall and transport.go's gated LogCall (which only fires for reads), so a blocked mutation attempt left no audit trail. registerSDKTool now routes every write-tool error through LogDenied (fan-out and tag-scope refusals, by gate name) or LogFailure (everything else), keeping the successful-write path logging its own resolved server count exactly as before. Also close a live scope gap surfaced while reviewing this: POST /api/keys/:id/assign called services.AssignKey with an unscoped GetServer lookup, so a tag-restricted token could assign a key to a server outside its restriction. The handler now resolves the target through GetServerScoped first, matching its sibling revoke route, and the route is recorded in serverScopedRoutes. --- server/internal/api/handlers.go | 8 +- server/internal/api/serverscope.go | 10 +++ server/internal/mcp/audit.go | 36 +++++++- server/internal/mcp/tools_write.go | 8 +- server/internal/mcp/transport.go | 58 +++++++++---- server/internal/mcp/transport_test.go | 115 ++++++++++++++++++++++++++ 6 files changed, 213 insertions(+), 22 deletions(-) create mode 100644 server/internal/mcp/transport_test.go diff --git a/server/internal/api/handlers.go b/server/internal/api/handlers.go index 42dd5db..3e2f153 100644 --- a/server/internal/api/handlers.go +++ b/server/internal/api/handlers.go @@ -623,6 +623,7 @@ func deleteKey(c *gin.Context) { // @Router /keys/{id}/assign [post] func assignKey(c *gin.Context) { keyID := c.Param("id") + instanceID := auth.InstanceID(c) var body struct { ServerID string `json:"server_id" binding:"required"` } @@ -631,7 +632,12 @@ func assignKey(c *gin.Context) { return } - a, err := services.AssignKey(auth.InstanceID(c), keyID, body.ServerID) + if _, err := services.GetServerScoped(instanceID, body.ServerID, auth.ServerScope(c)); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "server not found"}) + return + } + + a, err := services.AssignKey(instanceID, keyID, body.ServerID) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index 3c0bfe2..d58885f 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -50,6 +50,16 @@ var serverScopedRoutes = map[string]bool{ // assignment on a server outside its scope. "DELETE /api/keys/:id/assign/:serverId": true, + // assignKey resolves body.ServerID through GetServerScoped before calling + // services.AssignKey, which itself uses the unscoped GetServer — so a + // restricted token can no longer assign a key to a server outside its + // scope by naming it in the request body. This route's path carries + // neither "server" nor "console", so it is not swept in by + // serverTouchingRoutes and this entry is not boot-enforced; it is kept + // here anyway as the record of a considered decision, matching its + // sibling revoke route. + "POST /api/keys/:id/assign": true, + // Creating a server has no server to filter yet. "POST /api/servers": false, // The agent's own enrolment routes authenticate as the agent, not as a diff --git a/server/internal/mcp/audit.go b/server/internal/mcp/audit.go index 9f75285..572489e 100644 --- a/server/internal/mcp/audit.go +++ b/server/internal/mcp/audit.go @@ -14,10 +14,32 @@ import ( // the whole fleet on one badly phrased instruction. const FanOutLimit = 25 +// Gate names for a write tool's own policy refusals, on top of GateMCPScope +// and GateResourceScope in registry.go. These name a decision this package +// made deliberately, so a human reading audit_logs can tell "the agent was +// stopped by policy" from "the agent tried and the machine failed". +const ( + GateFanOut = "fan_out" + GateTagScope = "tag_selector" +) + // ErrConfirmRequired is returned to the model as a tool error it can act on: // it says what would have happened and how to proceed deliberately. var ErrConfirmRequired = errors.New("confirmation required") +// ErrOutOfScope wraps a write tool's refusal to act because the resolved (or, +// for run_workflow, the workflow's configured) targets are not entirely +// within the calling token's tag restriction. Handlers wrap this rather than +// returning a bare error so registerSDKTool can tell a scope refusal apart +// from an ordinary service failure and audit it as GateTagScope. +var ErrOutOfScope = errors.New("targets outside token scope") + +// logEvent is services.LogEvent behind a package variable so tests can +// observe what would have been audited without a live database connection — +// services.LogEvent talks straight to Mongo via db.Col, which panics on a nil +// client outside a real boot. +var logEvent = services.LogEvent + // CheckFanOut refuses a write that would touch more servers than FanOutLimit // unless the call passed confirm:true. func CheckFanOut(count int, args map[string]any) error { @@ -84,12 +106,22 @@ func LogCall(c Caller, t Tool, args map[string]any, servers int) { if servers > 0 { detail += fmt.Sprintf(", %d server(s) affected", servers) } - services.LogEvent(c.InstanceID, "mcp.tool_call", c.TokenName, "", "", detail) + logEvent(c.InstanceID, "mcp.tool_call", c.TokenName, "", "", detail) } // LogDenied records a refusal and which gate refused, which is what turns "the // agent said it could not" into a diagnosable event. func LogDenied(c Caller, toolName, gate string) { - services.LogEvent(c.InstanceID, "mcp.tool_denied", c.TokenName, "", "", + logEvent(c.InstanceID, "mcp.tool_denied", c.TokenName, "", "", fmt.Sprintf("tool %s refused by %s", toolName, gate)) } + +// LogFailure records a write tool call that reached a service and that +// service returned an error — as opposed to LogDenied, which records a +// policy refusal that never reached one. Distinguishing the two in +// audit_logs is what lets a human reading it tell "the agent was stopped" +// from "the agent tried and the machine failed". +func LogFailure(c Caller, t Tool, args map[string]any, err error) { + logEvent(c.InstanceID, "mcp.tool_failed", c.TokenName, "", "", + fmt.Sprintf("tool %s (%s) failed: %v", t.Name, SummariseArgs(args), err)) +} diff --git a/server/internal/mcp/tools_write.go b/server/internal/mcp/tools_write.go index abfb2fd..2d7be3e 100644 --- a/server/internal/mcp/tools_write.go +++ b/server/internal/mcp/tools_write.go @@ -78,7 +78,7 @@ func init() { } scopedTargets, err := services.ResolveTargetsScoped(c.InstanceID, wf.TargetServerIDs, wf.TargetTags, c.TokenScope) if err != nil || len(scopedTargets) != len(allTargets) { - return nil, fmt.Errorf("no servers visible to this token matched the request") + return nil, fmt.Errorf("%w: no servers visible to this token matched the request", ErrOutOfScope) } if err := CheckFanOut(len(scopedTargets), args); err != nil { @@ -162,7 +162,7 @@ func init() { ids := stringSliceArg(args, "server_ids") targets, err := services.ResolveTargetsScoped(c.InstanceID, ids, tagArg(args), c.TokenScope) if err != nil { - return nil, fmt.Errorf("no servers visible to this token matched the request") + return nil, fmt.Errorf("%w: no servers visible to this token matched the request", ErrOutOfScope) } if err := CheckFanOut(len(targets), args); err != nil { return nil, err @@ -209,7 +209,7 @@ func init() { ids := stringSliceArg(args, "server_ids") targets, err := services.ResolveTargetsScoped(c.InstanceID, ids, tagArg(args), c.TokenScope) if err != nil { - return nil, fmt.Errorf("no servers visible to this token matched the request") + return nil, fmt.Errorf("%w: no servers visible to this token matched the request", ErrOutOfScope) } if err := CheckFanOut(len(targets), args); err != nil { return nil, err @@ -272,7 +272,7 @@ func init() { ids := stringSliceArg(args, "server_ids") targets, err := services.ResolveTargetsScoped(c.InstanceID, ids, tagArg(args), c.TokenScope) if err != nil { - return nil, fmt.Errorf("no servers visible to this token matched the request") + return nil, fmt.Errorf("%w: no servers visible to this token matched the request", ErrOutOfScope) } if err := CheckFanOut(len(targets), args); err != nil { return nil, err diff --git a/server/internal/mcp/transport.go b/server/internal/mcp/transport.go index 5a5cdf7..5df7c97 100644 --- a/server/internal/mcp/transport.go +++ b/server/internal/mcp/transport.go @@ -71,24 +71,52 @@ func registerSDKTool(srv *sdk.Server, tool Tool, caller Caller) { Name: tool.Name, Description: tool.Description, }, func(ctx context.Context, req *sdk.CallToolRequest, args map[string]any) (*sdk.CallToolResult, any, error) { - if ok, gate := Allowed(tool, caller); !ok { - LogDenied(caller, tool.Name, gate) - return nil, nil, toolError(gate, tool) - } - - out, err := tool.Handler(ctx, caller, args) - if err != nil { - return nil, nil, err - } - if !tool.Write { - // Write tools log their own call with a resolved server count, - // which this layer cannot know. - LogCall(caller, tool, args, 0) - } - return nil, out, nil + return callTool(ctx, tool, caller, args) }) } +// callTool is the gate check, dispatch and audit write registerSDKTool wraps +// onto the SDK's call signature. It is a separate function — rather than the +// closure body inline — so it can be exercised directly in tests without +// standing up an sdk.Server and driving a real MCP request through it. +func callTool(ctx context.Context, tool Tool, caller Caller, args map[string]any) (*sdk.CallToolResult, any, error) { + if ok, gate := Allowed(tool, caller); !ok { + LogDenied(caller, tool.Name, gate) + return nil, nil, toolError(gate, tool) + } + + out, err := tool.Handler(ctx, caller, args) + if err != nil { + // A write tool's own handler never gets a chance to audit its own + // refusal or failure — it returns before reaching its LogCall, and + // unlike a successful write, this layer does not know a resolved + // server count to pass along anyway. So every write failure is + // audited here instead: a policy refusal (fan-out or tag scope) as + // mcp.tool_denied naming the gate, everything else as + // mcp.tool_failed, so a human reading audit_logs can tell "the agent + // was stopped" from "the agent tried and the machine failed". Read + // tools are unaffected — a failed read was never going to change + // anything and carries no gate to name. + if tool.Write { + switch { + case errors.Is(err, ErrConfirmRequired): + LogDenied(caller, tool.Name, GateFanOut) + case errors.Is(err, ErrOutOfScope): + LogDenied(caller, tool.Name, GateTagScope) + default: + LogFailure(caller, tool, args, err) + } + } + return nil, nil, err + } + if !tool.Write { + // Write tools log their own call with a resolved server count, which + // this layer cannot know. + LogCall(caller, tool, args, 0) + } + return nil, out, nil +} + // toolError explains a refusal in words the model can act on. A transport-level // failure would be invisible to it; a tool error is something it can read and // relay to its user. diff --git a/server/internal/mcp/transport_test.go b/server/internal/mcp/transport_test.go new file mode 100644 index 0000000..f6434dc --- /dev/null +++ b/server/internal/mcp/transport_test.go @@ -0,0 +1,115 @@ +package mcp + +import ( + "context" + "testing" +) + +// TestRefusedWriteIsAudited exercises the real dispatch path (callTool, which +// registerSDKTool wraps) for a write tool whose handler refuses the call +// before it ever reaches its own LogCall — a fan-out refusal, in this case, +// which run_workflow, apply_updates, update_agent and assign_key all reach +// the same way via CheckFanOut. The refusal must still produce an audit row: +// a blocked mutation attempt is the single most audit-worthy event a write +// tool produces, and until this test the only thing recording it was the +// tool's own success path. +func TestRefusedWriteIsAudited(t *testing.T) { + var got []string + restore := logEvent + logEvent = func(instanceID, eventType, actor, serverID, keyID, details string) { + got = append(got, eventType+": "+details) + } + defer func() { logEvent = restore }() + + tool := Tool{ + Name: "test_write_tool", + Write: true, + Scope: "servers:write", + Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { + return nil, CheckFanOut(200, args) + }, + } + + caller := Caller{Scopes: []string{"mcp:write", "servers:write"}} + + _, _, err := callTool(context.Background(), tool, caller, nil) + if err == nil { + t.Fatal("callTool() = nil error, want the fan-out refusal") + } + + if len(got) != 1 { + t.Fatalf("logEvent called %d times, want exactly 1 audit row for the refusal; got %v", len(got), got) + } + if want := "mcp.tool_denied: "; len(got[0]) < len(want) || got[0][:len(want)] != want { + t.Errorf("audit row %q does not record a denial", got[0]) + } +} + +// TestOutOfScopeWriteIsAudited covers the other write refusal shape: a +// service-layer ErrOutOfScope wrap, as apply_updates/update_agent/assign_key +// return when ResolveTargetsScoped finds nothing this token may touch. +func TestOutOfScopeWriteIsAudited(t *testing.T) { + var got []string + restore := logEvent + logEvent = func(instanceID, eventType, actor, serverID, keyID, details string) { + got = append(got, eventType) + } + defer func() { logEvent = restore }() + + tool := Tool{ + Name: "test_scoped_tool", + Write: true, + Scope: "servers:write", + Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { + return nil, ErrOutOfScope + }, + } + + caller := Caller{Scopes: []string{"mcp:write", "servers:write"}} + + if _, _, err := callTool(context.Background(), tool, caller, nil); err == nil { + t.Fatal("callTool() = nil error, want the scope refusal") + } + + if len(got) != 1 || got[0] != "mcp.tool_denied" { + t.Errorf("audit events = %v, want exactly one mcp.tool_denied row", got) + } +} + +// TestServiceFailureIsAuditedDistinctly makes sure a write tool failing for a +// reason that is not a policy refusal — the underlying service call itself +// erroring — is still audited, but as mcp.tool_failed rather than +// mcp.tool_denied, so a human reading audit_logs can tell the two apart. +func TestServiceFailureIsAuditedDistinctly(t *testing.T) { + var events []string + restore := logEvent + logEvent = func(instanceID, eventType, actor, serverID, keyID, details string) { + events = append(events, eventType) + } + defer func() { logEvent = restore }() + + tool := Tool{ + Name: "test_failing_tool", + Write: true, + Scope: "servers:write", + Handler: func(ctx context.Context, c Caller, args map[string]any) (any, error) { + return nil, errFakeServiceFailure + }, + } + + caller := Caller{Scopes: []string{"mcp:write", "servers:write"}} + + if _, _, err := callTool(context.Background(), tool, caller, nil); err == nil { + t.Fatal("callTool() = nil error, want the service failure") + } + + if len(events) != 1 || events[0] != "mcp.tool_failed" { + t.Errorf("audit events = %v, want exactly one mcp.tool_failed row", events) + } +} + +var errFakeServiceFailure = &fakeError{"the dispatcher refused the command"} + +type fakeError struct{ msg string } + +func (e *fakeError) Error() string { return e.msg }