Gateway: enforce allowed-tools filtering server-side on tools/list and tools/call#3333
Gateway: enforce allowed-tools filtering server-side on tools/list and tools/call#3333
Conversation
- Add isToolAllowed() helper to UnifiedServer that checks a tool against the server's configured Tools list - Filter tools during registerToolsFromBackend so non-allowed tools never appear in tools/list responses - Reject tools/call in callBackendTool for tools not in the allowed list, returning an MCP error with IsError:true - Log denied tool calls at WARN level via logger.LogWarn with truncated session ID using auth.TruncateSessionID - Add comprehensive tests: isToolAllowed unit tests, registration filtering tests, callBackendTool enforcement tests" Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/00582754-0d76-4bb6-a950-0cd6ff534f1b Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…, use client category for denied tool call warning Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/00582754-0d76-4bb6-a950-0cd6ff534f1b Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…oolAllowed and registerToolsFromBackend Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/00582754-0d76-4bb6-a950-0cd6ff534f1b Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR closes a security gap where agents could bypass client-side --allowed-tools by sending raw tools/call requests, by enforcing the configured per-server tool allow-list on the gateway side in both tools/list exposure and tools/call execution.
Changes:
- Precomputes per-server allowed-tool sets during
UnifiedServerinitialization for O(1) checks. - Enforces allow-list in
callBackendTool(returns an error result + 403 span status when blocked). - Filters backend tool registration so disallowed tools never appear in
tools/list, plus adds targeted tests.
Show a summary per file
| File | Description |
|---|---|
| internal/server/unified.go | Builds allowedToolSets and blocks disallowed tools early in callBackendTool. |
| internal/server/tool_registry.go | Filters backend tools/list results to only register/advertise allowed tools. |
| internal/server/tool_registry_test.go | Adds coverage ensuring registration/listing is filtered when allow-lists are configured. |
| internal/server/call_backend_tool_test.go | Adds coverage for allowed-tools enforcement behavior in callBackendTool. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
internal/server/call_backend_tool_test.go:380
- This test creates a UnifiedServer but doesn’t call Close(). Since NewUnified starts background goroutines (health monitor) and holds resources, please
defer us.Close()after NewUnified to avoid leaks between tests.
us, err := NewUnified(context.Background(), cfg)
require.NoError(err)
ctx := context.WithValue(context.Background(), SessionIDContextKey, "test-session")
internal/server/call_backend_tool_test.go:472
- This test initializes a UnifiedServer via NewUnified but never closes it. Add
defer us.Close()after successful construction to avoid goroutine/resource leaks during the test run.
us, err := NewUnified(context.Background(), cfg)
require.NoError(err)
ctx := context.WithValue(context.Background(), SessionIDContextKey, "sess-123")
result, _, callErr := us.callBackendTool(ctx, "s", "blocked", nil)
- Files reviewed: 4/4 changed files
- Comments generated: 2
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot make sure that allow-tools filtering has thorough unit and integration tests |
…utine leaks; fix error message wording and remove unused auth import - Add allowed_tools_integration_test.go with 11 tests covering tools/list filtering, tools/call blocking, routed mode, multiple servers, unrestricted mode, buildAllowedToolSets, and isToolAllowed - Add defer us.Close() in TestCallBackendTool_NoAllowedListPermitsAllTools and TestCallBackendTool_AllowedToolsError_MessageFormat (fixes goroutine leaks) - TestAllowedTools_CallBlocked_ReturnsError verifies the backend is NOT called when a tool is blocked - Remove unused 'auth' import from unified.go (no longer needed after log message wording change) Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/994b596a-0e4d-411b-a68a-76d4fd59fdb1 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…g to avoid double conversion Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/994b596a-0e4d-411b-a68a-76d4fd59fdb1 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Added a dedicated
Also fixed two missing Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/dfce2174-6f66-49d4-b73e-4189d4ba04f8 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…d tools/call (#3334) Agents with raw HTTP access to the gateway could bypass client-side `--allowed-tools` filters by directly sending `tools/call` JSON-RPC requests for tools they shouldn't be able to call. The existing `tools` field in `StdinServerConfig`/`ServerConfig` was parsed but never enforced at runtime. ## Changes ### Pre-computed allowed-tools sets (`unified.go`) - Added `allowedToolSets map[string]map[string]bool` to `UnifiedServer`, built once at init via `buildAllowedToolSets(cfg)` for O(1) per-call lookup - Added `isToolAllowed(serverID, toolName)` — returns `true` when no list is configured (unrestricted) ### Enforcement in `callBackendTool` (`unified.go`) Before any DIFC/guard work, rejects calls for tools not in the allowed set: - Returns `IsError: true` `CallToolResult` with a descriptive message - Sets OTEL span HTTP status to 403 - Logs at WARN with `logger.LogWarn("client", ...)` including the server ID ### tools/list defense-in-depth (`tool_registry.go`) During backend tool registration, non-allowed tools are filtered out — they never appear in `tools/list` responses and are never registered with the SDK server. ### Lint fix - Removed unused `sendUnifiedMCPRequest` and `parseSSEBody` helper functions from `allowed_tools_integration_test.go` (golangci-lint `unused` violations)
Agents with raw HTTP access to the gateway could bypass client-side
--allowed-toolsfilters by directly sendingtools/callJSON-RPC requests for tools they shouldn't be able to call. The existingtoolsfield inStdinServerConfig/ServerConfigwas parsed but never enforced at runtime.Changes
Pre-computed allowed-tools sets (
unified.go)allowedToolSets map[string]map[string]booltoUnifiedServer, built once at init viabuildAllowedToolSets(cfg)for O(1) per-call lookupisToolAllowed(serverID, toolName)— returnstruewhen no list is configured (unrestricted)Enforcement in
callBackendTool(unified.go)Before any DIFC/guard work, rejects calls for tools not in the allowed set:
IsError: trueCallToolResultwith a descriptive message ("tool %q is not in the allowed-tools list for this server")logger.LogWarn("client", ...)including the server IDtools/list defense-in-depth (
tool_registry.go)During backend tool registration, non-allowed tools are filtered out using the pre-computed
allowedToolSets— they never appear intools/listresponses and are never registered with the SDK server.Config usage
The existing
toolsfield on each MCP server entry is the allow-list:{ "mcpServers": { "github": { "type": "stdio", "container": "ghcr.io/github/github-mcp-server:latest", "tools": ["search_code", "get_file_contents", "list_issues"] } } }When
toolsis empty or absent, all tools remain accessible (no behavior change for existing configs).Testing
A dedicated
internal/server/allowed_tools_integration_test.goprovides 11 integration tests covering the full enforcement path end-to-end:IsError: truewith the backend verified to not receive the forwarded request; unrestricted server passes all toolsbuildAllowedToolSetswith multiple servers and empty/nil config;isToolAllowedwith real configAdditional unit tests in
call_backend_tool_test.goandtool_registry_test.gocover theisToolAllowedhelper,callBackendToolrejection, and registration filtering.