Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion gateway/mw_jsonrpc_access_control.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/http"

"github.com/TykTechnologies/tyk/internal/httpctx"
"github.com/TykTechnologies/tyk/internal/mcp"
"github.com/TykTechnologies/tyk/internal/middleware"
)

Expand Down Expand Up @@ -53,7 +54,7 @@ func (m *JSONRPCAccessControlMiddleware) ProcessRequest(w http.ResponseWriter, r
return nil, http.StatusOK
}

if checkAccessControlRules(accessDef.JSONRPCMethodsAccessRights, state.Method) {
if mcp.CheckAccessControlRules(accessDef.JSONRPCMethodsAccessRights, state.Method) {
writeJSONRPCAccessDenied(w, r, fmt.Sprintf("method '%s' is not available", state.Method))
return nil, middleware.StatusRespond
}
Expand Down
40 changes: 0 additions & 40 deletions gateway/mw_jsonrpc_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,48 +5,8 @@ import (

"github.com/TykTechnologies/tyk/internal/httpctx"
jsonrpcerrors "github.com/TykTechnologies/tyk/internal/jsonrpc/errors"
"github.com/TykTechnologies/tyk/regexp"
"github.com/TykTechnologies/tyk/user"
)

// checkAccessControlRules evaluates allow/block lists against a name.
// Returns true if the name is denied, false if permitted.
//
// Evaluation order:
// 1. Blocked is checked first — if matched, the request is denied.
// 2. If Allowed is non-empty and the name does not match any entry, the request is denied.
// 3. If both lists are empty, access is permitted.
func checkAccessControlRules(rules user.AccessControlRules, name string) bool {
for _, pattern := range rules.Blocked {
if matchPattern(pattern, name) {
return true
}
}

if len(rules.Allowed) == 0 {
return false
}

for _, pattern := range rules.Allowed {
if matchPattern(pattern, name) {
return false
}
}

return true
}

// matchPattern tests name against a regex pattern anchored with ^...$, enforcing full-match semantics.
// Uses the tyk/regexp package which caches compiled patterns.
// Falls back to exact-string comparison if the pattern is not valid regex.
func matchPattern(pattern, name string) bool {
re, err := regexp.Compile("^(?:" + pattern + ")$")
if err != nil {
return pattern == name
}
return re.MatchString(name)
}

// writeJSONRPCAccessDenied writes a JSON-RPC 2.0 error response for access-denied cases.
// Delegates to jsonrpcerrors.WriteJSONRPCError for consistent response shape and HTTP→JSON-RPC
// error code mapping across all error paths in the gateway.
Expand Down
126 changes: 0 additions & 126 deletions gateway/mw_jsonrpc_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,134 +8,8 @@ import (
"github.com/stretchr/testify/require"

"github.com/TykTechnologies/tyk/internal/httpctx"
"github.com/TykTechnologies/tyk/user"
)

func TestCheckAccessControlRules(t *testing.T) {
tests := []struct {
name string
rules user.AccessControlRules
input string
wantErr bool
}{
// Empty rules — everything passes
{"empty rules, any input passes", user.AccessControlRules{}, "anything", false},
{"empty rules, empty input passes", user.AccessControlRules{}, "", false},

// Blocked only
{"blocked exact match", user.AccessControlRules{Blocked: []string{"tools/list"}}, "tools/list", true},
{"blocked no match", user.AccessControlRules{Blocked: []string{"tools/list"}}, "tools/call", false},
{"blocked regex match", user.AccessControlRules{Blocked: []string{"delete_.*"}}, "delete_user", true},
{"blocked regex no match", user.AccessControlRules{Blocked: []string{"delete_.*"}}, "create_user", false},
{"blocked suffix regex", user.AccessControlRules{Blocked: []string{".*_admin"}}, "user_admin", true},
{"blocked suffix regex no match", user.AccessControlRules{Blocked: []string{".*_admin"}}, "user_create", false},
{"blocked multiple patterns, first matches", user.AccessControlRules{Blocked: []string{"delete_.*", "reset_.*"}}, "delete_all", true},
{"blocked multiple patterns, second matches", user.AccessControlRules{Blocked: []string{"delete_.*", "reset_.*"}}, "reset_config", true},
{"blocked multiple patterns, none match", user.AccessControlRules{Blocked: []string{"delete_.*", "reset_.*"}}, "get_data", false},

// Allowed only
{"allowed exact match", user.AccessControlRules{Allowed: []string{"tools/call"}}, "tools/call", false},
{"allowed not in list", user.AccessControlRules{Allowed: []string{"tools/call"}}, "tools/list", true},
{"allowed regex match", user.AccessControlRules{Allowed: []string{"get_.*"}}, "get_weather", false},
{"allowed regex no match", user.AccessControlRules{Allowed: []string{"get_.*"}}, "set_config", true},
{"allowed multiple patterns, first matches", user.AccessControlRules{Allowed: []string{"get_.*", "list_.*"}}, "get_weather", false},
{"allowed multiple patterns, second matches", user.AccessControlRules{Allowed: []string{"get_.*", "list_.*"}}, "list_users", false},
{"allowed multiple patterns, none match", user.AccessControlRules{Allowed: []string{"get_.*", "list_.*"}}, "delete_all", true},

// Both — blocked takes precedence
{"deny precedence over allow (exact)",
user.AccessControlRules{Blocked: []string{"reset_system"}, Allowed: []string{"reset_system"}},
"reset_system", true},
{"deny precedence over allow (regex)",
user.AccessControlRules{Blocked: []string{".*_system"}, Allowed: []string{"reset_.*"}},
"reset_system", true},
{"allowed passes when not in blocked",
user.AccessControlRules{Blocked: []string{"delete_.*"}, Allowed: []string{"get_.*", "delete_.*"}},
"get_weather", false},

{"alternation: allowed prefix match", user.AccessControlRules{Allowed: []string{"get_.*|set_.*"}}, "get_weather", false},
{"alternation: allowed second branch", user.AccessControlRules{Allowed: []string{"get_.*|set_.*"}}, "set_config", false},
{"alternation: denied non-matching", user.AccessControlRules{Allowed: []string{"get_.*|set_.*"}}, "delete_all", true},
{"alternation: allowed spurious prefix leak", user.AccessControlRules{Allowed: []string{"get_.*|set_.*"}}, "bad_prefix_set_foo", true},
{"alternation: blocked spurious trailing leak", user.AccessControlRules{Blocked: []string{"admin|debug"}}, "prefix_debug", false},

// Edge cases
{"pattern with URI chars", user.AccessControlRules{Allowed: []string{"file:///public/.*"}}, "file:///public/readme.md", false},
{"pattern with URI chars, no match", user.AccessControlRules{Allowed: []string{"file:///public/.*"}}, "file:///secret/keys.txt", true},
{"invalid regex falls back to exact, exact match", user.AccessControlRules{Blocked: []string{"[invalid"}}, "[invalid", true},
{"invalid regex falls back to exact, no match", user.AccessControlRules{Blocked: []string{"[invalid"}}, "something", false},
{"method name with slash", user.AccessControlRules{Blocked: []string{"tools/list"}}, "tools/list", true},
{"method regex with slash", user.AccessControlRules{Allowed: []string{"resources/.*"}}, "resources/read", false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
denied := checkAccessControlRules(tt.rules, tt.input)
if tt.wantErr {
assert.True(t, denied, "expected denied for input %q", tt.input)
} else {
assert.False(t, denied, "expected allowed for input %q", tt.input)
}
})
}
}

func TestMatchPattern(t *testing.T) {
tests := []struct {
pattern string
name string
want bool
}{
// Exact matches
{"get_weather", "get_weather", true},
{"get_weather", "get_weather_v2", false}, // anchored: no partial match
{"tools/call", "tools/call", true},
{"tools/call", "tools/list", false},

// Prefix wildcards
{"get_.*", "get_weather", true},
{"get_.*", "set_config", false},
{"resources/.*", "resources/read", true},
{"resources/.*", "prompts/get", false},

// Suffix wildcards
{".*_admin", "user_admin", true},
{".*_admin", "user_create", false},
{".*_delete", "record_delete", true},

// Regex alternation
{"get_.*|set_.*", "get_weather", true},
{"get_.*|set_.*", "set_config", true},
{"get_.*|set_.*", "delete_all", false},
{"get_.*|set_.*", "bad_prefix_set_foo", false},
{"get_.*|set_.*", "get_weather_extra", true},
{"a|b", "xb", false},
{"a|b", "ax", false},
{"a|b", "a", true},
{"a|b", "b", true},

// URI patterns with slashes
{"file:///.*", "file:///repo/README", true},
{"file:///public/.*", "file:///public/readme.md", true},
{"file:///public/.*", "file:///secret/keys.txt", false},

// Invalid regex — falls back to exact comparison
{"[invalid", "[invalid", true},
{"[invalid", "something", false},

// Empty cases
{".*", "", true},
{"", "", true},
}

for _, tt := range tests {
t.Run(tt.pattern+"_vs_"+tt.name, func(t *testing.T) {
got := matchPattern(tt.pattern, tt.name)
assert.Equal(t, tt.want, got, "matchPattern(%q, %q)", tt.pattern, tt.name)
})
}
}

func TestWriteJSONRPCAccessDenied_WithState(t *testing.T) {
r := httptest.NewRequest("POST", "/mcp", nil)
state := &httpctx.JSONRPCRoutingState{
Expand Down
2 changes: 1 addition & 1 deletion gateway/mw_mcp_access_control.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (m *MCPAccessControlMiddleware) ProcessRequest(w http.ResponseWriter, r *ht
}

rules := rulesForPrimitiveType(accessDef.MCPAccessRights, state.PrimitiveType)
if checkAccessControlRules(rules, state.PrimitiveName) {
if mcp.CheckAccessControlRules(rules, state.PrimitiveName) {
writeJSONRPCAccessDenied(w, r,
fmt.Sprintf("%s '%s' is not available", state.PrimitiveType, state.PrimitiveName))
return nil, middleware.StatusRespond
Expand Down
133 changes: 133 additions & 0 deletions gateway/res_handler_mcp_list_filter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
package gateway

import (
"bytes"
"io"
"net/http"
"strconv"
"strings"

"github.com/TykTechnologies/tyk/internal/httpctx"
"github.com/TykTechnologies/tyk/internal/mcp"
"github.com/TykTechnologies/tyk/user"
)

// MCPListFilterResponseHandler filters MCP list responses (tools/list, prompts/list,
// resources/list, resources/templates/list) to show only primitives the consumer
// is authorized to see based on their MCPAccessRights allow/block lists.
type MCPListFilterResponseHandler struct {
BaseTykResponseHandler
}

// Base returns the base handler for middleware decoration.
func (h *MCPListFilterResponseHandler) Base() *BaseTykResponseHandler {
return &h.BaseTykResponseHandler
}

// Name returns the handler name for logging and debugging.
func (h *MCPListFilterResponseHandler) Name() string {
return "MCPListFilterResponseHandler"
}

// Init initializes the handler with the given spec.
func (h *MCPListFilterResponseHandler) Init(_ any, spec *APISpec) error {
h.Spec = spec
return nil
}

// Enabled returns true only for MCP APIs.
func (h *MCPListFilterResponseHandler) Enabled() bool {
return h.Spec.IsMCP()
}

// HandleResponse filters MCP list responses based on session access rights.
func (h *MCPListFilterResponseHandler) HandleResponse(_ http.ResponseWriter, res *http.Response, req *http.Request, ses *user.SessionState) error {
state := httpctx.GetJSONRPCRoutingState(req)
if state == nil {
return nil
}

listCfg := h.listConfig(state.Method)
if listCfg == nil {
return nil
}

// Skip SSE streaming responses — list methods return JSON, but guard against
// Streamable HTTP servers that might choose to respond with text/event-stream.
// Reading the full body of an SSE stream would block indefinitely.
if ct := res.Header.Get("Content-Type"); strings.HasPrefix(ct, "text/event-stream") {
return nil
}

rules := h.rulesForAPI(ses, listCfg)
if rules.IsEmpty() {
return nil
}

Check warning on line 65 in gateway/res_handler_mcp_list_filter.go

View check run for this annotation

probelabs / Visor: performance

performance Issue

The `HandleResponse` function reads the entire response body into memory via `readAndCloseBody` (`io.ReadAll`) before filtering. For very large list responses, this can cause significant memory allocations and put pressure on the garbage collector. While the author's performance analysis notes this is acceptable for the current expected scale, it presents a potential scalability risk if list sizes grow significantly.
Raw output
For future optimization, consider replacing the full-body read with a streaming JSON parser. This would allow filtering the list without holding the entire response in memory, albeit at the cost of increased implementation complexity. No immediate action is required, but this should be monitored.

body, err := readAndCloseBody(res)
if err != nil || len(body) == 0 {
return nil //nolint:nilerr // fail-open: pass through on read error

Check warning on line 69 in gateway/res_handler_mcp_list_filter.go

View check run for this annotation

probelabs / Visor: quality

logic Issue

The handler silently ignores errors from reading the response body. When `readAndCloseBody` fails, it replaces the response body with an empty one, and this handler proceeds without signaling an error. The client receives a `200 OK` with an empty body, which can mask underlying network issues between the gateway and the upstream service. Additionally, the comment "pass through on read error" is misleading, as the body is replaced, not passed through.
Raw output
Log the error from `readAndCloseBody` to improve observability. If silent failure is the desired behavior, update the comment to accurately reflect that the response body is replaced with an empty one on a read error. A more robust approach would be to propagate the error to the middleware chain, potentially resulting in a 5xx response to the client, making the failure visible.
}

newBody, ok := mcp.FilterJSONRPCBody(body, listCfg, rules)
if !ok {
res.Body = io.NopCloser(bytes.NewReader(body))
return nil
}

res.Body = io.NopCloser(bytes.NewReader(newBody))
res.ContentLength = int64(len(newBody))
res.Header.Set("Content-Length", strconv.Itoa(len(newBody)))

return nil
}

// rulesForAPI extracts the access control rules for the current API from the
// session, returning empty rules if the session has no applicable restrictions.
func (h *MCPListFilterResponseHandler) rulesForAPI(ses *user.SessionState, cfg *mcp.ListFilterConfig) user.AccessControlRules {
if ses == nil {
return user.AccessControlRules{}
}

accessDef, ok := ses.AccessRights[h.Spec.APIID]
if !ok || accessDef.MCPAccessRights.IsEmpty() {
return user.AccessControlRules{}
}

return cfg.RulesFrom(accessDef.MCPAccessRights)
}

// listConfig returns the filter configuration for a given JSON-RPC method,
// or nil if the method is not a filterable list method.
func (h *MCPListFilterResponseHandler) listConfig(method string) *mcp.ListFilterConfig {
switch method {
case mcp.MethodToolsList:
return mcp.ListFilterConfigs["tools"]

Check warning on line 105 in gateway/res_handler_mcp_list_filter.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The `listConfig` method in `MCPListFilterResponseHandler` duplicates the logic of mapping a method name to a filter configuration. A similar mapping is needed in the SSE hook, which infers the configuration from response keys. This suggests an opportunity for a more centralized and reusable mapping mechanism within the `internal/mcp` package.
Raw output
Create a new function in the `internal/mcp` package, such as `GetListConfigForMethod(method string) *ListFilterConfig`, which contains the switch statement. This would centralize the method-to-config mapping, making it reusable and the single source of truth. The `MCPListFilterResponseHandler` can then call this new function, reducing code duplication and improving maintainability.
case mcp.MethodPromptsList:
return mcp.ListFilterConfigs["prompts"]
case mcp.MethodResourcesList:
return mcp.ListFilterConfigs["resources"]
case mcp.MethodResourcesTemplatesList:
return mcp.ListFilterConfigs["resourceTemplates"]
default:
return nil
}
}

// readAndCloseBody reads the full response body and closes it. On success the
// caller owns the returned bytes; the original body is always closed.
// Returns (nil, nil) when the body is nil.
func readAndCloseBody(res *http.Response) ([]byte, error) {
if res.Body == nil {
return nil, nil
}

body, err := io.ReadAll(res.Body)

Check warning on line 125 in gateway/res_handler_mcp_list_filter.go

View check run for this annotation

probelabs / Visor: security

security Issue

The response handler reads the entire response body into memory using `io.ReadAll` before filtering. If the upstream service returns a very large JSON response for a list method, this could lead to excessive memory consumption in the gateway, potentially causing a denial-of-service (DoS) condition due to OOM errors. While list responses are not typically huge, there is no protection against an abnormally large payload.
Raw output
To mitigate the risk of resource exhaustion, consider replacing `io.ReadAll` with a streaming JSON parser (e.g., `json.Decoder`). This would allow processing the JSON structure token by token, filtering items as they are read from the stream, and writing the filtered output to a new buffer without holding the entire response body in memory at once. This approach is more resilient to large payloads.
res.Body.Close()
if err != nil {
res.Body = io.NopCloser(bytes.NewReader(nil))
return nil, err
}

return body, nil
}
Loading
Loading